typeshed icon indicating copy to clipboard operation
typeshed copied to clipboard

openpyxl: Workbook opened in readonly mode should return `ReadOnlyCell | EmptyCell` from `worksheet.rows`

Open jamesbeith opened this issue 2 years ago • 5 comments

Currently the type hints for worksheet.rows returns Cell objects. However, opening a workbook in readonly mode results in ReadOnlyCell or EmptyCell objects being returned.

>>> workbook = openpyxl_excel_reader.load_workbook(file, read_only=True)
>>> worksheet = workbook.worksheets[0]  # openpyxl.worksheet._read_only.ReadOnlyWorksheet
>>> [row for row in worksheet.rows]
[
    (<ReadOnlyCell 'Sheet1'.A1>, <EmptyCell>, <ReadOnlyCell 'Sheet1'.C1>),
    (<ReadOnlyCell 'Sheet1'.A2>, <EmptyCell>, <ReadOnlyCell 'Sheet1'.C2>),
]

jamesbeith avatar Mar 25 '23 00:03 jamesbeith

The problem is that .worksheets always return Worksheet and does not ever return _ReadOnlyWorksheet: https://github.com/python/typeshed/blob/main/stubs/openpyxl/openpyxl/workbook/workbook.pyi#L61

And load_workbook always returns Workbookhttps://github.com/python/typeshed/blob/a544b75320e97424d2d927605316383c755cdac0/stubs/openpyxl/openpyxl/reader/excel.pyi#L50

So, we don't have enough typing information right now to make this work.

Ideas?

sobolevn avatar Mar 27 '23 10:03 sobolevn

Could we use @overload and Literal for the load_workbook() function, where if read_only is the literal True then we instead return some kind of new ReadOnlyWorkbook type which itself returns _ReadOnlyWorksheet from .worksheets (and then ReadOnlyCell | EmptyCell for it's rows). Is that a feasible idea?

https://mypy.readthedocs.io/en/stable/literal_types.html#literal-types

jamesbeith avatar Mar 28 '23 23:03 jamesbeith

Yes, seems ok to me :) Do you want to work on this?

sobolevn avatar Mar 29 '23 07:03 sobolevn

Do you want to work on this?

@sobolevn I'll aim to take a look 👍🏼

jamesbeith avatar Apr 04 '23 09:04 jamesbeith

Do note that this problem goes one step further: ReadOnlyWorksheet cell methods come from Worksheet, so you'd have to specialize ReadOnlyWorksheet's cell-related method types on top of making Workbook generic.

Avasam avatar Apr 04 '24 17:04 Avasam