FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix pos-only params in `tkinter` module by sobolevn · Pull Request #11506 · python/typeshed · GitHub

Repository navigation

Fix pos-only params in tkinter module - #11506

Merged
AlexWaygood merged 2 commits into
mainfrom
pos-only-tkinter
Feb 29, 2024
Merged

AlexWaygood merged 2 commits into
mainfrom
pos-only-tkinter

Conversation

Copy link
Copy Markdown
Member

Same as #11505

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

This comment was marked as resolved.

AlexWaygood merged commit 7b4c751 into main Feb 29, 2024
AlexWaygood deleted the pos-only-tkinter branch February 29, 2024 09:02

Akuli commented Feb 29, 2024

Copy link
Copy Markdown
Collaborator

This is less readable. What problem does this solve?

AlexWaygood commented Feb 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

This is less readable. What problem does this solve?

See conversation in #11505 (comment). I had the same initial reaction, but pyright's error is, strictly speaking, correct here. It's invalid to have a pos-only parameter following a pos-or-keyword parameter, and stubtest correctly complains about the alternative solution I thought of.

Akuli commented Feb 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

I still don't see a practical problem being solved. Type checkers will generally tolerate small issues in stubs as long as they can understand what the stubs are trying to say. On the other hand, IDE users will not be happy trying to decipher the new Unpack contraption.

Copy link
Copy Markdown
Member

I'd be okay reverting if we add test cases to make sure type checkers understand that it's valid at runtime to pass default using a keyword argument. But note that there's no way of directly translating what we had before this PR into PEP-570 syntax -- with PEP-570, all parameters before the / are positional-only. And we're hoping to soon move to PEP-570 everywhere (#11250).

Akuli commented Feb 29, 2024

Copy link
Copy Markdown
Collaborator

it's valid at runtime to pass default using a keyword argument

It's not. This method cannot be called so that you pass a keyword argument for default. If you try to do it, python will complain that you're passing two values for it:

>>> import tkinter
>>> root = tkinter.Tk()
>>> root.wm_iconphoto('lol', default=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Wm.wm_iconphoto() got multiple values for argument 'default'

I now see that the old way to write the stub with default and __image1 is wrong/misleading, but I still don't like what this PR did.

AlexWaygood commented Feb 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

it's valid at runtime to pass default using a keyword argument

It's not. This method cannot be called so that you pass a keyword argument for default. If you try to do it, python will complain that you're passing two values for it:

Got it -- my mistake. FWIW, I did test locally, but obviously not thoroughly enough :)

In that case, we should be able to simply update the signature to the much-simpler

def wm_iconphoto(self, default: bool, image1: _PhotoImageLike | str, /, *args: _PhotoImageLike | str) -> None: ...

Akuli commented Feb 29, 2024

Copy link
Copy Markdown
Collaborator

FWIW, I did test locally, but obviously not thoroughly enough :)

To be honest, the whole wm_iconphoto() method is a mess. A positional-only boolean parameter is just a bad idea for readability, but it's too late to change it now: that would break every tkinter program that sets a custom icon for the app.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL