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

Add type annotations, minor cleanups #2044

Merged
merged 35 commits into from
Jul 26, 2023

Conversation

NMertsch
Copy link
Contributor

@NMertsch NMertsch commented Jul 22, 2023

Describe your changes in detail

  • diff Unify how App/Window/Widget IDs are created and documented, and ensures that the ID is always a string.
  • diff Add type annotations to user-facing classes
  • Refs Add type annotations for callbacks #2042 by adding typing.Protocols for callback functions
  • Fix a few typos (a/an, missing comma, ...)

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

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks like a good cleanup; there's a minor tweak that can be made to the docstrings. The doctoring on app.py looks like it was historically wrong because it didn't have a type annotation.

core/src/toga/widgets/base.py Outdated Show resolved Hide resolved
core/src/toga/window.py Outdated Show resolved Hide resolved
@NMertsch NMertsch changed the title Unify ID creation Add type annotations, minor cleanups Jul 23, 2023
@NMertsch NMertsch marked this pull request as draft July 23, 2023 09:02
@NMertsch NMertsch force-pushed the unify-id-creation branch from 8752aef to 96f662f Compare July 23, 2023 11:46
@NMertsch NMertsch force-pushed the unify-id-creation branch from 96f662f to 7b5ff67 Compare July 23, 2023 12:09
@freakboy3742
Copy link
Member

FYI: The Linux CI failure will be fixed by #2052. The Android CI failure will be fixed by beeware/briefcase#1380.

@NMertsch NMertsch marked this pull request as ready for review July 24, 2023 08:07
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Thanks for these improvements. I've made a couple of minor tweaks to wording and naming of the new protocols, but otherwise this looks great - and will make the task of rolling out the analogous changes to other widget classes a lot easier (as we now have a template to follow).

Out of interest - did you log the bug with Sphinx around the :rtype: spelling error? I found a little more detail on this one - if you have any :param: or :returns: declaration, the "spelling error" goes away (e.g., adding a :param widget: declaration to the OnPress handler fixes the spelling error with the None return type.

@freakboy3742 freakboy3742 merged commit d9dc14e into beeware:main Jul 26, 2023
@NMertsch
Copy link
Contributor Author

Looks great, thank you!

@freakboy3742 yes, I reported it here: sphinx-contrib/spelling#216

It seems to be caused by the autoprocol plugin. Won't have time to investigate it in the next days, though.

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