Additional type hints added to stub openpyxl#9764
Conversation
Updated classes `Workbook`, `Worksheet`, `Cell`. Type checking works in VS Code
|
@Avasam FYI |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Just one last point, then we're good to go!
| def get_named_range(self, name): ... | ||
| def remove_named_range(self, named_range) -> None: ... | ||
| def named_styles(self) -> list[str]: ... | ||
| def get_named_ranges(self) -> list[DefinedName] | tuple[DefinedName]: ... |
There was a problem hiding this comment.
| def get_named_ranges(self) -> list[DefinedName] | tuple[DefinedName]: ... | |
| def get_named_ranges(self) -> list[DefinedName] | tuple[DefinedName, ...]: ... |
Remember that tuple[T] means "a single-element tuple where the sole element is of type T". You want tuple[T, ...] if you want to signify "a tuple of arbitrary length, where all elements are of type T".
We could also possibly simplify this by just doing Sequence[DefinedName] (with Sequence imported from collections.abc)?
| def get_named_ranges(self) -> list[DefinedName] | tuple[DefinedName]: ... | |
| def get_named_ranges(self) -> Sequence[DefinedName]: ... |
There was a problem hiding this comment.
Agreed on the tuple[DefinedName, ...].
collections.abc.Sequence is broader than just list and tuple. It also includes str and bytes: https://docs.python.org/3/glossary.html#term-sequence
I don't think collections.abc.Sequence would be appropriate.
There was a problem hiding this comment.
collections.abc.Sequenceis broader than justlistandtuple. It also includesstrandbytes: https://docs.python.org/3/glossary.html#term-sequence
str and bytes are indeed both sequences, correct — but neither is a subtype of Sequence[DefinedName]. str is a subtype of Sequence[str], and bytes is a subtype of Sequence[int]. So if I were writing this PR, I would favour the simpler approach here :)
But your approach isn't wrong by any means, and I don't want to be too picky :)
There was a problem hiding this comment.
2 things here I just noticed (sorry it's been a while that I've looked at openpyxl)
1: descriptors are not meant to be used directly for type hints. Descriptors are assigned to ClassVars of classes subtyping Serializable. Serializable members act as properties where the setter uses the descriptor for runtime vlaidation of the setter
2. Whilst the Sequence descriptor allows both list and tuple (https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.1/openpyxl/descriptors/sequence.py#L18), DefinedNameList.definedName specifically needs to be sliceable and appendable (at least in 3.0, this doesn't seem to be the case anymore for 3.1) https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/3.0/openpyxl/workbook/defined_name.py#L207-209, the latter meaning it can't be a tuple.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Thanks @ArnabRollin for the initial PR, and thanks @mynhardtburger for finishing it! 🥳
| @property | ||
| def columns(self) -> Generator[Cell, None, None]: ... | ||
| def set_printer_settings( | ||
| self, paper_size: int | None, orientation: None | Literal["default", "portrait", "landscape"] |
There was a problem hiding this comment.
Another case for PyCQA/flake8-pyi#175 and PyCQA/flake8-pyi#283 -like checks in Flake8-PYI.
4 participants
This is a continuation of the #9487 stale PR, with the recommended changes implemented.