Skip to content
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

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

freakboy3742
Copy link
Member

Discovered in the review of #2252.

SplitContainer's content attribute was typed as a tuple. 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:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@freakboy3742 freakboy3742 marked this pull request as ready for review June 11, 2024 06:50
@freakboy3742 freakboy3742 requested a review from rmartin16 June 11, 2024 06:51
@freakboy3742 freakboy3742 mentioned this pull request Jun 11, 2024
4 tasks
Copy link
Member

@rmartin16 rmartin16 left a 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.

core/src/toga/widgets/splitcontainer.py Outdated Show resolved Hide resolved
core/src/toga/widgets/splitcontainer.py Outdated Show resolved Hide resolved
core/src/toga/widgets/splitcontainer.py Outdated Show resolved Hide resolved
core/src/toga/widgets/splitcontainer.py Outdated Show resolved Hide resolved
core/src/toga/widgets/splitcontainer.py Outdated Show resolved Hide resolved
@freakboy3742
Copy link
Member Author

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 😏.

Look... everyone can have a bad day 🤪 .

Additionally, I did initially pursue typing this as a Sequence...but mypy doesn't support them in specific sizes.

For my own edification - I'm interested in the motivation for the change from Sequence to list. I went with Sequence because I thought the general approach was for typing to be as generic as possible; Sequence guarantees __getitem__, __len__, and __iter__, which (AFAICT) are the only methods we need. Am I missing something here?

@rmartin16
Copy link
Member

Additionally, I did initially pursue typing this as a Sequence...but mypy doesn't support them in specific sizes.

For my own edification - I'm interested in the motivation for the change from Sequence to list. I went with Sequence because I thought the general approach was for typing to be as generic as possible; Sequence guarantees __getitem__, __len__, and __iter__, which (AFAICT) are the only methods we need. Am I missing something here?

As for the use of list vs Sequence, or more generally, concrete vs abstract, it's mostly just about inputs vs outputs. When defining a type to ingest, use an abstract protocol; that especially avoids issues with completely custom classes. When defining a type to output or return, use the concrete type of the return value.

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 OnSelectHandler, for instance. In reality, the return type of the getter is toga.handlers.WrappedHandlerT. (Although, this return type is really less important anyway without support for async and generator handlers in the typing.)

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.

@freakboy3742
Copy link
Member Author

Additionally, I did initially pursue typing this as a Sequence...but mypy doesn't support them in specific sizes.

For my own edification - I'm interested in the motivation for the change from Sequence to list. I went with Sequence because I thought the general approach was for typing to be as generic as possible; Sequence guarantees __getitem__, __len__, and __iter__, which (AFAICT) are the only methods we need. Am I missing something here?

As for the use of list vs Sequence, or more generally, concrete vs abstract, it's mostly just about inputs vs outputs. When defining a type to ingest, use an abstract protocol; that especially avoids issues with completely custom classes. When defining a type to output or return, use the concrete type of the return value.

Ah - that makes sense. I mentally scanned your updates as s/Sequence/list, but I see now it's only the return types that have been updated in your suggestions.

@freakboy3742 freakboy3742 requested a review from rmartin16 June 11, 2024 23:45
Copy link
Member

@rmartin16 rmartin16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@freakboy3742 freakboy3742 merged commit cd7ca2f into beeware:main Jun 11, 2024
31 of 34 checks passed
@freakboy3742 freakboy3742 deleted the splitcontainer-sequence branch June 11, 2024 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants