Skip to content

create _tkinter stub #4372

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

Merged
merged 9 commits into from
Aug 8, 2020
Merged

create _tkinter stub #4372

merged 9 commits into from
Aug 8, 2020

Conversation

Akuli
Copy link
Collaborator

@Akuli Akuli commented Jul 29, 2020

checks a checkbox in #1019

I thought I'd do some smaller tkinter PRs because #4363 doesn't seem to be getting merged in a while (needs conversation)

@Akuli
Copy link
Collaborator Author

Akuli commented Jul 29, 2020

How do I tell stubtest that an attribute comes from a __getattr__? By overloading __getattr__ with Literals?

@hauntsaninja
Copy link
Collaborator

stubtest is okay with attributes generally, because those are often defined on instances, so it can't get at them without instantiating. It does assume methods are not dynamically created though.
You have two options:
a) you can annotate your dynamic methods as attributes, eg: eval: Callable[[str], Any] (or for more complicated things, of type callback protocol). Depending on what getattr is doing, this might actually be more honest.
b) just add it to the stubtest whitelists (which I think is fine doing in this case). for an example in a similar vein, we whitelisted all of ast.NodeVisitor's visit_* methods.

Really appreciate all your improvements!

@Akuli
Copy link
Collaborator Author

Akuli commented Jul 31, 2020

ready for review

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 8, 2020

How about putting the methods to a class named _Something and inheriting the two classes from that? This way there would be no copy/pasta.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Sorry for the delay and thank you for the much needed improvements to Tcl/Tk! Looks mostly good to me, with one nit below.

But on a general note: Stubs should only contain brief stub-related comments. Further documentation is out of scope for stubs and should be part of the documentation and/or implementation. If you want to document how you determined a certain type or possible problems for the reviewer to look out for, the best way to do that is with inline comments in the PR.


# These come from dir(_tkinter). I don't know when actually using any of this
# would be convenient
ALL_EVENTS: int = ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit (also applies to assignments below): = ... is redundant and should be left out.

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 8, 2020

I made the comments more concise.

I can see how to apply these commenting guidelines to some of my comments in #4363, for example, and I will change that PR appropriately next. But sometimes I really want people working on the stubs to see something, even if they're adding something new rather than modifying something that I wrote. Can I still use comments for that?

Edit: nevermind, I think I understood this now

@srittau srittau merged commit 954ce8c into python:master Aug 8, 2020
@Akuli Akuli deleted the _tkinter branch August 8, 2020 12:16
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.

3 participants