Additional type hints added to stub openpyxl
#9487
Conversation
Updated classes `Workbook`, `Worksheet`, `Cell`. Type checking works in VS Code
This comment has been minimized.
This comment has been minimized.
|
(I'm not a typeshed maintainer, but I contribute a lot here) There's a big semantic difference between "Unknown" and "Any". One says to be careful because the type isn't checked. The other says you can truly use anything. Nowadays typeshed uses To fix the issue on pyright/pylance. You could:
To resolve the issue with mypy. Since you already installed types-openpyxl, make sure that you are using mypy from your virtual environment. I am not personally familiar with pipenv, but it's possible that pyright comes with a copy of typeshed's third-party stubs. mypy doesn't and you have to install them manually. openpyxl is also very special. All those class members using descriptors are actually properties in disguise. With runtime type-checking and type coercion. I'm currently working on merging the stubs from https://github.com/microsoft/python-type-stubs/tree/main/openpyxl |
I usually like to put my code in strict mode. But, when I found the type stubs, I thought they were a bit incomplete... So I updated these. |
They are indeed :) And contributions are very welcome! Thank you for the PR! |
|
Diff from mypy_primer, showing the effect of this PR on open source code: pandas (https://github.com/pandas-dev/pandas)
+ pandas/io/excel/_openpyxl.py:73: error: Argument 1 to "load_workbook" has incompatible type "IO[bytes]"; expected "str" [arg-type]
+ pandas/io/excel/_openpyxl.py:101: error: Argument 1 to "save" of "Workbook" has incompatible type "IO[bytes]"; expected "str" [arg-type]
+ pandas/io/excel/_openpyxl.py:554: error: Argument 1 to "load_workbook" has incompatible type "Union[Union[str, PathLike[str]], ReadBuffer[bytes]]"; expected "str" [arg-type]
|
| @@ -19,60 +25,60 @@ TYPE_ERROR: str | |||
| TYPE_FORMULA_CACHE_STRING: str | |||
| VALID_TYPES: Incomplete | |||
|
|
|||
| def get_type(t, value): ... | |||
| def get_time_format(t): ... | |||
| def get_type(t: Any, value: Any) -> Any: ... | |||
There was a problem hiding this comment.
tlooks like it has to be a class of some kind, due to this line here, where it's used as a key in a dictionary that only has classes as keys: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/cell/cell.py#L73valuecould be literally any object, so it's better to useobjecthere rather thanAny.Anyis an "escape hatch" in the type system for things that are truly inexpressible. It's very unsafe, so it's better to useobjectwhenever possible.- The function always either returns a
strorNone.
| def get_type(t: Any, value: Any) -> Any: ... | |
| def get_type(t: type, value: object) -> str | None: ... |
| def get_type(t, value): ... | ||
| def get_time_format(t): ... | ||
| def get_type(t: Any, value: Any) -> Any: ... | ||
| def get_time_format(t: Any) -> Any: ... |
There was a problem hiding this comment.
tis clearly expected to be adatetimeobject, due to thisTIME_FORMATS.get(t)call here (TIME_FORMATSonly hasdatetimeobjects as keys): https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/cell/cell.py#L78- The function always returns the value that's associated with the key passed in, and the values of
TIME_FORMATSare allstrs. So the function always returns astr.
| def get_time_format(t: Any) -> Any: ... | |
| def get_time_format(t: datetime) -> str: ... |
| worksheet: Worksheet, | ||
| row: int | None = ..., | ||
| column: int | None = ..., | ||
| value: str | float | int | datetime | None = ..., |
There was a problem hiding this comment.
PEP 484 specifies that functions annotated as accepting float should also accept int, so int is redundant in this union
| value: str | float | int | datetime | None = ..., | |
| value: str | float | datetime | None = ..., |
| @property | ||
| def value(self): ... | ||
| def value(self) -> str | float | int | datetime | None: ... |
There was a problem hiding this comment.
| def value(self) -> str | float | int | datetime | None: ... | |
| def value(self) -> str | float | datetime | None: ... |
| @value.setter | ||
| def value(self, value) -> None: ... | ||
| def value(self, value: str | float | int | datetime | None) -> None: ... |
There was a problem hiding this comment.
| def value(self, value: str | float | int | datetime | None) -> None: ... | |
| def value(self, value: str | float | datetime | None) -> None: ... |
| def delete_cols(self, idx: int, amount: int = ...) -> None: ... | ||
| def move_range(self, cell_range: CellRange, rows: int = ..., cols: int = ..., translate: bool = ...) -> None: ... | ||
| @property | ||
| def print_title_rows(self) -> Any: ... |
There was a problem hiding this comment.
Please leave unannotated if you don't know the type.
| @property | ||
| def print_title_cols(self): ... | ||
| def print_title_cols(self) -> Any: ... |
There was a problem hiding this comment.
Please leave unannotated if you don't know the type.
| def print_titles(self) -> Any: ... | ||
| @property | ||
| def print_area(self): ... | ||
| def print_area(self) -> Any: ... |
There was a problem hiding this comment.
Please leave unannotated if you don't know the type.
| @print_area.setter | ||
| def print_area(self, value) -> None: ... | ||
| def print_area(self, value: str) -> None: ... |
There was a problem hiding this comment.
This is incorrect, it also accepts an iterable of strings: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/worksheet/worksheet.py#L883-891
| @print_title_cols.setter | ||
| def print_title_cols(self, cols) -> None: ... | ||
| def print_title_cols(self, cols: int) -> None: ... |
There was a problem hiding this comment.
This is incorrect, it accepts str | None, but not int: https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/worksheet/worksheet.py#L855-863
|
@ArnabRollin are you still working on this, or can I offer my help to implement the change requests? |
|
@mynhardtburger would you like to pick up where @ArnabRollin picked off? You'd be very welcome to! (@ArnabRollin, you'll be listed as a co-author, of course.) |
|
@AlexWaygood I would like to try... This is my first time contributing to a public project. |
Yes, that would be the process! :) |
If you do, please tag me in the new PR so I can update the description in #9511 |
Reason for change
The VS Code Python static type-checker does not recognise
Here is an example:
Mypy doesn't seem to find the
types-openpyxlpackage(Working-with-Excel) -> pipenv install types-openpyxl(Working-with-Excel) -> mypy app.pyPlease point out if I am wrong anywhere.