Skip to content
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

Explicitly type attribute sender in messages. #1922

Closed
wants to merge 11 commits into from
Closed

Conversation

rodrigogiraoserrao
Copy link
Contributor

This will fix #1813.

This PR also removes some superfluous event attributes, like Button.Pressed.button or Input.Changed.input.
These are noted in the CHANGELOG.

Finally, this PR makes DataTable events generics over the same data type as the DataTable itself, akin to how Tree does it.

I typechecked all my changes and I did not introduce new type errors.
(Or rather, I did but I fixed them before opening the PR! 🤪 )

@rodrigogiraoserrao rodrigogiraoserrao changed the title Fix 1813 Explicitly type attribute sender in messages. Mar 2, 2023
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

After some thought, I don't think we can merge this.

More accurate typing is generally a win, but sender is an internal detail. It just so happens that sender is the thing you interacted with for most widgets, but we don't guarantee that. In the future, maybe something else sends the message. Or we have different child widgets send the event, but we want them to be treated in the same way by a parent. In that case sender may be of different types.

There's also the issue that event.sender.id could be anything, but event.input.id is self documenting.

@@ -3,7 +3,9 @@
import functools
from dataclasses import dataclass
from itertools import chain, zip_longest
from multiprocessing import Event
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you have some kind of auto-import adder in your IDE. I think you should turn it off. Pretty sure we don't use multiprocessing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't know how this ended up here! 😅
I might have some auto-import thing turned on.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might have some auto-import thing turned on.

I think it's part of pylance/pyright; it ha a habit of doing the same thing via eglot in Emacs. Generally it only does the import if you pick an auto-imported completion (in Emacs it's generally easy to see where it's comping from so you can avoid it), and normally I noticed if it's managed to sneak one in when it comes to reviewing the commit in magit (atomic commits for the win here, of course).

What I've yet to find is how to tell pylance/pyright to turn it off. There is apparently a setting (python.analysis.autoImportCompletions) but I've yet to figure out a non-project-specific way of configuring it.

from operator import itemgetter
from tkinter import E
Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely don't need this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how this ended up here! 😅

@willmcgugan willmcgugan closed this Mar 2, 2023
@rodrigogiraoserrao rodrigogiraoserrao deleted the fix-1813 branch March 9, 2023 12:01
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.

[Typing] Add type hint to sender in widget messages?
3 participants