Skip to content

Fix incomplete tkinter.commondialog stub #14340

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.

Sign up for GitHub

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 4 commits into from
Jun 26, 2025
Merged

Fix incomplete tkinter.commondialog stub #14340

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
379c731
Complete `tkinter.commondialog` stub
MarcellPerger1 Jun 20, 2025
c893658
Merge branch 'main' into improve-tkinter-commondialog
MarcellPerger1 Jun 26, 2025
e73fa45
Fix stubtest error
MarcellPerger1 Jun 26, 2025
7510140
Add comment explaining use of `Any`
MarcellPerger1 Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions stdlib/tkinter/commondialog.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
from _typeshed import Incomplete
from collections.abc import Mapping
from typing import ClassVar
from tkinter import Misc
from typing import Any, ClassVar

__all__ = ["Dialog"]

class Dialog:
command: ClassVar[str | None]
master: Incomplete | None
options: Mapping[str, Incomplete]
def __init__(self, master=None, **options) -> None: ...
def show(self, **options): ...
master: Misc | None
# Types of options are very dynamic. They depend on the command and are
# sometimes changed to a different type.
options: Mapping[str, Any]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We now recommend adding a comment for each use of Any. For example:

Suggested change
options: Mapping[str, Any]
# Types of options are very dynamic. They depend on the command and are
# sometimes changed to a different type.
options: Mapping[str, Any]

IMO adding just one comment like this is enough for the whole file, since all Any uses are for things named options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now recommend adding a comment for each use of Any

Separate issue, but it might be worth mentioning this in the CONTRIBUTING.md file or somewhere more visible (or is there already some place with a collection of recommendations like this that I'm just missing?)

Copy link
Member

@brianschubert brianschubert Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

it might be worth mentioning this in the CONTRIBUTING.md file

Hmm, there was a mention, but it was removed as part of #13332. It might make sense to re-add it and perhaps other parts that were more about typeshed policy than style

def __init__(self, master: Misc | None = None, **options: Any) -> None: ...
def show(self, **options: Any): ...
Copy link
Contributor

@GameRoMan GameRoMan Jun 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Should the return type of show also be Any or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think the return type should be Any. Currently, it is implicitly Any (as it has no return type annotations) but it would probably be better to make is explicit.
(I just forgot to update the return type to Any)