[openpyxl] Fix stubs for openpyxl.descriptors.sequence.UniqueSequence.seq_types #14725
Conversation
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
| @@ -38,7 +38,7 @@ class Sequence(Descriptor[_ContainerT]): | |||
|
|
|||
| # `_T` is the type of the elements in the sequence. | |||
| class UniqueSequence(Sequence[set[_T]]): | |||
| seq_types: tuple[type[list[_T]], type[tuple[_T, ...]], type[set[_T]]] | |||
| seq_types: tuple[type, ...] # defaults to `list`, `tuple`, `set` | |||
There was a problem hiding this comment.
Just commented on the issue, but I think this makes the stub worse and it's better to ignore the error.
There was a problem hiding this comment.
Should this be adjusted in Sequence then as well? And i think in principle this can be set to anything though, right?
UniqueSequence just includes a set in the default?
There was a problem hiding this comment.
I also think that the stubs in this particular case are best left unchanged, since any modification would reduce accuracy. It should be sufficient to change comment in stubtest_allowlist.txt entry explaining why we decided not to adjust these types."
There was a problem hiding this comment.
As example (on pyright playgroun):
stuff1 = UniqueSequenceOld[int](seq_types=(list, tuple, set, frozenset), container=frozenset)
stuff2 = UniqueSequenceNew[int](seq_types=(list, tuple, set, frozenset), container=frozenset)
I think this is valid at runtime. But to allow it we would need to have unique sequence as:
class UniqueSequence(Sequence[[_ContainerT]]):
seq_types: tuple[type, ...]
container: type
which i guess feels weird?
So this would be a "useability vs purity" thing?
There was a problem hiding this comment.
Oh, I didn't realize the types could be overwritten. If so, using tuple[type, ...] does make more sense.
There was a problem hiding this comment.
I think so at the very least:
https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/default/openpyxl/descriptors/base.py#L17
https://foss.heptapod.net/openpyxl/openpyxl/-/blob/branch/default/openpyxl/descriptors/sequence.py#L11
I think "UniqueSequence" just changes the defaults. So changing them would definitely make them more correct in the general sense i think. But it would also make them pretty useless in the (common?) case where you keep the defaults.
Linking #14718 for visibility there