Skip to content

Conversation

@Akuli
Copy link
Collaborator

@Akuli Akuli commented Feb 29, 2024

The whole method is just weird. The default argument is a boolean cannot be passed as a keyword argument:

>>> 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'

Also, the default argument must be given. So calling this method looks like window.wm_iconphoto(False, some_image) or window.wm_iconphoto(False, image1, image2, image3).

Having a required positional-only boolean parameter is dumb, but tkinter does it, and it can't be changed without breaking backwards compatibility.

See also the discussion in #11506.

@Akuli Akuli changed the title Simplify wm_iconphoto() tkinter: Simplify wm_iconphoto() Feb 29, 2024
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

@github-actions
Copy link
Contributor

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

@Akuli Akuli merged commit 0ad004a into main Feb 29, 2024
@Akuli Akuli deleted the Akuli-patch-1 branch February 29, 2024 11:45
@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 29, 2024

We should do the same thing for os.execl and os.execlp, which were changed in #11505. file is pos-or-keyword according to inspect.signature, so we'll need to add a stubtest allowlist. But you can't actually pass it as a keyword parameter, same as for wm_iconphoto:

>>> import os
>>> os.execl(file='foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<frozen os>", line 548, in execl
ValueError: execv() arg 2 must not be empty
>>> os.execl('bar', file='foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: execl() got multiple values for argument 'file'

@AlexWaygood
Copy link
Member

We should do the same thing for os.execl and os.execlp, which were changed in #11505. file is pos-or-keyword according to inspect.signature, so we'll need to add a stubtest allowlist. But you can't actually pass it as a keyword parameter, same as for wm_iconphoto:

>>> import os
>>> os.execl(file='foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<frozen os>", line 548, in execl
ValueError: execv() arg 2 must not be empty
>>> os.execl('bar', file='foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: execl() got multiple values for argument 'file'

Cc. @sobolevn

@sobolevn
Copy link
Member

I think the proper solution is to use *args: Unpack, actually. It is harder to read, but at the same time it is more correct semantically: our goal is to specify the *args structure and Unpack does just that. But, it is a bit of a mess, I agree :(

@AlexWaygood
Copy link
Member

I think the proper solution is to use *args: Unpack, actually. It is harder to read, but at the same time it is more correct semantically: our goal is to specify the *args structure and Unpack does just that. But, it is a bit of a mess, I agree :(

The current signature after #11505 specifies that file can be passed as a keyword argument to those os functions. Look at the REPL snippet I posted above: it cannot be passed as a keyword argument, or you get a runtime error.

@sobolevn
Copy link
Member

@AlexWaygood mypy correctly handles this case: https://mypy-play.net/?mypy=latest&python=3.12&gist=6133cabb92b165e44e240d341383fe31

It is a general rule: you cannot pass keyword args if you also pass *args args.

@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 29, 2024

@AlexWaygood mypy correctly handles this case: https://mypy-play.net/?mypy=latest&python=3.12&gist=6133cabb92b165e44e240d341383fe31

It is a general rule: you cannot pass keyword args if you also pass *args args.

Hmm, fair enough. I still feel like the signature for the os functions is now unnecessarily ugly though, which isn't great for users of IDEs, who'll now have to decipher Unpack in autocomplete suggestions etc

@sobolevn
Copy link
Member

I always forget about IDE users, since I don't use one :)

danieleades pushed a commit to danieleades/typeshed that referenced this pull request Mar 1, 2024
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.

4 participants