Skip to content

Add options to tkinter widgets #4363

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 54 commits into from
Aug 17, 2020

Conversation

Akuli
Copy link
Collaborator

@Akuli Akuli commented Jul 25, 2020

Tkinter options now have type hints that describe which values are valid when setting the options. For example, tkinter.Label(relief='sunkenn') now creates a mypy error. The correct types for the options come from Tk's manual pages as described in __init__.pyi. This affects __init__, config, configure, cget, __setitem__ and __getitem__ methods because those are the methods that deal with widget options.

Looking up the value of an option always returns Any, because tkinter doesn't make much effort to ensure that the return values have consistent type. In practice, they can be pretty much anything in funny corner cases, and figuring out which of the possible types actually come up frequently enough for type hints would be a lot of work.

In tkinter, widget['foo'] = bar and widget.config(foo=bar) do the same thing, but they type-check differently: the widget['foo'] = bar syntax allows 'foo' to be any string (e.g. a variable, not necessarily a Literal) and bar to be any object, while widget.config(foo=bar) checks the existence of the option and the type of bar. Similarly, cget takes a Literal argument but __getitem__ takes a string. This is done because:

  • It halves the amount of required copy/pasta in stubs files.
  • The type of a variable defined with foo = 'sunken' is str rather than Literal['sunken'], so fixing code that does widget.config(relief=foo) can be done without a # type: ignore comment by changing it to widget['relief'] = foo.
  • Similarly, setting bar = 'relief' and doing widget[bar] = 'sunken' works.

This also includes a script that checks whether the types of widget options actually work at runtime (stubtest doesn't work well with the generous **kwargs usage in tkinter) and the travis configuration to make that part of the CI build. Usually it takes about 22 seconds to run, which is a lot quicker than any of the other CI things. The script creates tkinter widgets, parses a small subset of type hint syntax with ast, creates possible values of the options based on those type hints and tries to set the options of the widgets.

Some classes in tkinter/ttk.py inherit from other classes and break the Liskov substitution principle by not having all the widget options of the base class. In those cases, I made the stubs not inherit and instead used method = BaseClass.method.

I added a --verbose option to tests/pytype_test.py for finding pytype issues more easily, but that's disabled in the travis config.

This pull request contains a lot less copy/pasta than I initially thought it would contain, but there's still a lot of it. The copy/pasta is quite readable even though I used a script to initially generate it. The script is not included in this pull request because I think the copy/pasta is maintainable as is, and learning to use my script to maintain it would be more work than with copy/pasta. I added comments about the trickiest options to the stubs.

@Akuli
Copy link
Collaborator Author

Akuli commented Jul 26, 2020

@Hawk777 What do you think about the difference between __setitem__ and config? Is it too confusing or surprising?

@Akuli
Copy link
Collaborator Author

Akuli commented Jul 26, 2020

Rebasing on top of latest master because there's not yet any conversations that could be lost by doing that

@Akuli Akuli force-pushed the tkinter-options-noscript-pr-squashed branch from 7c1d1f4 to cbeab82 Compare July 26, 2020 13:39
@Hawk777
Copy link
Contributor

Hawk777 commented Jul 26, 2020

I’m uncertain how I feel about the configure-vs-__setitem__ and cget-vs-__getitem__ thing. It does have a nice technical benefit that, if you’re not doing any indirection (i.e. you’re using literals), you can just always use configure and cget and you get awesome type checking, and if you are doing indirection, well, there’s still a way to do that. I think there are two big concerns though. First, as far as I can tell (and I could very well be wrong as I’m not a Typeshed expert), it seems like most of Typeshed is developed with the intention of reflecting reality as accurately as possible within the type hint system; in other words, Typeshed seems to try not to be super-opinionated about how people write their code, rather some theoretical “perfect Typeshed” would be one where every valid program is accepted and every invalid program is rejected. This is definitely a change of direction, since it’s saying, “even though it’s totally legal to pass a non-literal string to cget, even though it will work properly at runtime, and even though we could have made your program typecheck if you do that, we chose to reject it instead.”

Second, I worry about discoverability; where would all these Typeshed-specific rules be documented? You have this stronger type checking, but how will people ever know that it exists? Certainly not from reading the Python documentation for tkinter, which explicitly states that __getitem__ and cget are exactly the same thing. What’s even going to prompt people to look for special tkinter-specific, Typeshed-specific documentation where they learn that they can get this extra type-checking?

Neither of these is necessarily a reason to reject, but I think they need to be thought through.

@Akuli
Copy link
Collaborator Author

Akuli commented Jul 27, 2020

Tkinter stubs have other things that need documentation too:

I think having to look up these things from typeshed pull request and issues history would be pretty bad. Has there been similar "type hints of this library need documentation" problems with other libraries in the past? If so, has the documentation went to docs.python.org, someone's unofficial username.github.io site, comments in stub files, or just nowhere?

@Akuli
Copy link
Collaborator Author

Akuli commented Jul 27, 2020

Looking at tkinter documentation on docs.python.org, there's already a big list of references that people should look at, and maybe another item in the list for using type hints with tkinter would be a good idea. Who should maintain that though? I don't think that akuli.github.io/tkinter-type-hints or something is a good idea.

@Akuli
Copy link
Collaborator Author

Akuli commented Jul 27, 2020

Maybe I'll mention another idea: put a markdown file into this typeshed repo and link to that from python docs. Maybe that would be a little weird, but it would be very easy to keep it updated as pull requests get merged to typeshed.

@Akuli Akuli mentioned this pull request Jul 29, 2020
Akuli added 3 commits July 31, 2020 13:08
Previously I allowed them to be numbers too, which could be useful for
letting the user e.g. choose a port number from a list of acceptable
ports numbers. I think that doesn't come up as often as would often as
accidentally passing numbers for values. More importantly, this makes
the type more readable in e.g. IDE autocompletion or mypy errors.
@Akuli
Copy link
Collaborator Author

Akuli commented Aug 2, 2020

Even though this pull request requires some conversation before getting merged, I'd like to make progress with this. The tkinter stubs won't be usable to me before this (or some similar pull request) gets merged.

@srittau @hauntsaninja It's been quiet for a while, and CONTRIBUTING.md seems to say that it's ok to ping after "more than a few days".

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 17, 2020

I created #4453 to fix _FontDescription because that's already on master and not really related to this PR. Everything else is done.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks again for your hard work and patience! You've been exceptionally thorough; the test script and the real world testing both give a lot of confidence. I did some more spot checking and things look good. If there are more improvements to be made, let's make them on master :-)

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