-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add missing tkinter submodules #4558
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
Conversation
Maybe @Akuli has time to take a look :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! I noticed a few small things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the code again and spotted more small things.
from typing import ClassVar, Optional | ||
|
||
class DndHandler: | ||
root: ClassVar[Optional[Tk]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be a ClassVar
. It's set in __init__
.
def invoke(self, name: str) -> None: ... | ||
|
||
class ComboBox(TixWidget): | ||
def __init__(self, master: Optional[tkinter.Widget] = ..., cnf: Dict[str, Any] = ..., **kw: Any) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most tkinter.Widget
usages in this file should be tkinter.Misc
instead. See the # Quick guide for figuring out which widget class to choose
comment in tkinter/__init__.pyi
.
self, | ||
master: Optional[Misc], | ||
text: str = ..., | ||
buttons: List[str] = ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be typing.Sequence
instead of List
, or maybe one of the SupportsFoo
classes in _typeshed
. It needs to have __getitem__
and __len__
, so it could also be e.g. a tuple or a collections.deque
. (Note that tkinter._TkinterSequence
is the right type for many things in tkinter, but not for this one.)
def add_child(self, parent: Optional[str] = ..., cnf: Dict[str, Any] = ..., **kw: Any) -> tkinter.Widget: ... | ||
def anchor_set(self, entry: str) -> None: ... | ||
def anchor_clear(self) -> None: ... | ||
# FIXME: Overload, certain combos return, others don't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Any
as the return type for things like this to avoid having to write assert return_value is not None
whenever the return value is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to merge this due to the upcoming restructuring. Remaining issues can be fixed later.
Adds the missing tkinter stubs from the list in #4545. Would love a review by someone who is more confident in their tkinter than me, if anyone is familiar with it.