-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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
bpo-32839: Add after_info to tkinter #5664
Conversation
Lib/tkinter/__init__.py
Outdated
@@ -763,15 +763,31 @@ def after_cancel(self, id): | |||
|
|||
Identifier returned by after or after_idle must be | |||
given as first parameter.""" | |||
if id is None: | |||
return |
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.
This makes no sense to me. id
is required and AFAKI should not be passed as None
. Do after
or after_idle
ever return 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.
Please see my following comment with the Tcl code. I added this because I'm not sure if the original comments about the tuple changing were accurate. I think it was a result of id being None vs it having a value.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I pushed a rebase and got the Appveyor error. I'm working on the other concerns about the |
Misc/NEWS.d/next/Library/2018-02-13-10-02-54.bpo-32839.McbVz3.rst
Outdated
Show resolved
Hide resolved
Cheryl, can you explain the closing? I still think adding after_info would be good. |
The only reported error, only on Travis, on line 191 is an extra item, 'after#8' (the 'timer' of line 180, I presume), in the tuple returned from .after_info. (The Travis job page somehow disables copying with current Firefox.) On this system, with the tcl/tk used, the preceding root.update() did not remove the 'timer' callback from the list. I restarted the failing job to see if the failure is repeatable on Travis. Answer: yes. |
FWIW, I also think adding |
Sorry about closing this prematurely. I had thought there hadn't been much interest in adding the functionality when I originally proposed it, so I was just doing some housekeeping. Good to know there is interest. Having said that, the tests pass locally, so it may be tricky to get them to pass. I have a |
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.
Please expand the NEWS entry, add a What's New entry apply and test the code suggestion.
The PR looks mostly good to me and I want to land it in 3.9.
Misc/NEWS.d/next/Library/2018-02-13-10-02-54.bpo-32839.McbVz3.rst
Outdated
Show resolved
Hide resolved
This PR is stale because it has been open for 30 days with no activity. |
This reverts commit 5b33773.
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 expanded the NEWS entry and added the What's New entry. My last code suggestions turned out to be wrong.
LGTM.
I am presuming that my old comments are obsolete. |
https://bugs.python.org/issue32839