-
-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct the typing of SplitContainer content to be list/Sequence #2638
Correct the typing of SplitContainer content to be list/Sequence #2638
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm mostly ambivalent on whether this should be a list
or a tuple
....but I did find commit 87b8500 interesting in the context of this PR 😏. Additionally, I did initially pursue typing this as a Sequence
...but mypy doesn't support them in specific sizes.
Couple of minor changes and we're good to go.
Look... everyone can have a bad day 🤪 .
For my own edification - I'm interested in the motivation for the change from |
As for the use of Obvious example: def make_list(foo: Iterable[str]) -> list[str]:
return list(foo) I think our specific example here may be complicating this, though, by the fact it's a property getter/setter and the conflict between the functionality and documentation. For the functionality, the setter should use the abstract type and the getter should use the concrete type. However, that does mean the documentation will show the type of the property from the perspective of the getter; therefore, the concrete type. I think there is room to argue either is correct in the docs. This was also an issue for the handlers....but I compromised for the sake of the documentation. In those cases, I typed the return of the getters as So, anyway....that's my perspective. I think our use of properties may benefit from a more robust display in Sphinx to help differentiate how the getter and setter types are managed. That said, Python typing currently has a giant hole for property support anyway. |
Ah - that makes sense. I mentally scanned your updates as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
Discovered in the review of #2252.
SplitContainer's
content
attribute was typed as atuple
. Correct usage of tuple vs list is a hill I will die on :-) - and while there's a possible interpretation where SplitContainer content is considered to be N-position-sensitive content items (which would be consistent with a tuple), IMHO it's more natural to think of a list of content. Each item in the list has the same type, and the only unusual restriction is the content must have a length of 2.As further supporting evidence - the existing documentation refers to "A sequence of 2 elements".
PR Checklist: