bpo-32857: tkinter after_cancel to raise error if called with None by csabella · Pull Request #5701 · python/cpython

csabella · 2018-02-16T19:41:49Z

https://bugs.python.org/issue32857

…None

serhiy-storchaka · 2018-02-19T12:32:38Z

Lib/tkinter/test/test_tkinter/test_misc.py

+
+        # Cancel timer event.
+        (script, _) = root.tk.splitlist(root.tk.call('after', 'info', timer1))
+        self.assertIn(script, root._tclCommands)


_tclCommands is an implementation detail. It would be better to not use it in tests. Otherwise changing the implementation (it already was changed in the past) will break tests.

But you can test a side effect of the script by calling root.tk.call(script).

Thank you for this suggestion. I wasn't quite sure how to test that the script still existed after the cancel.

serhiy-storchaka · 2018-02-19T12:35:15Z

Lib/tkinter/test/test_tkinter/test_misc.py

+        with self.assertRaises(ValueError):
+            root.after_cancel(None)
+
+        # A non-existent id raises a TclError, which is caught in after_cancel.


The comment looks misleading. This is the test for after_cancel() which doesn't raise a TclError.

Actually I think that checking that root.tk.call('after', 'info', 'spam') raises a TclError is not needed. Using this command in after_cancel() is an implementation detail.

serhiy-storchaka · 2018-02-19T12:39:04Z

Lib/tkinter/test/test_tkinter/test_misc.py


+    def test_after_cancel(self):
+        root = self.root
+        timer1 = root.after(5000, lambda: 'break')


Make commands having a side effect (for example adding an item to the list) and check that after canceling events and calling update() they are not executed.

I made a counter within a callback function. Is that OK or is a list a better solution?

serhiy-storchaka · 2018-02-19T12:43:07Z

Lib/tkinter/test/test_tkinter/test_misc.py

+        (script, _) = root.tk.splitlist(root.tk.call('after', 'info', timer1))
+        self.assertIn(script, root._tclCommands)
+        root.after_cancel(timer1)
+        self.assertNotIn(script, root._tclCommands)


Here root.tk.call(script) should raise a TclError and shouldn't have a side effect.

Thanks! This is very helpful in my understanding.

serhiy-storchaka · 2018-02-20T11:00:54Z

Misc/NEWS.d/next/Library/2018-02-16-14-37-14.bpo-32857.-XljAx.rst

@@ -0,0 +1 @@
+:mod:`tkinter` change handling of ``None`` parameter call to after_cancel.


Could you please provide more useful information for readers? Like "after_cancel(None) now is error instead of canceling the first scheduled function."

And please add "Patch by your name."

miss-islington · 2018-03-04T10:41:49Z

Thanks @csabella for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

…e. (pythonGH-5701) (cherry picked from commit 74382a3) Co-authored-by: Cheryl Sabella <[email protected]>

bedevere-bot · 2018-03-04T10:42:57Z

GH-5972 is a backport of this pull request to the 3.7 branch.

miss-islington · 2018-03-04T10:43:52Z

Sorry, @csabella and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 74382a3f175ac285cc924a73fd758e8dc3cc41bb 2.7

…e. (pythonGH-5701) (cherry picked from commit 74382a3) Co-authored-by: Cheryl Sabella <[email protected]>

bedevere-bot · 2018-03-04T10:44:56Z

GH-5973 is a backport of this pull request to the 3.6 branch.

…e. (GH-5701) (cherry picked from commit 74382a3) Co-authored-by: Cheryl Sabella <[email protected]>

csabella · 2018-03-04T12:56:30Z

@serhiy-storchaka Would you like me to cherry pick this to 2.7?

serhiy-storchaka · 2018-04-18T17:29:59Z

Yes, it would be nice.

…e. (pythonGH-5701) (cherry picked from commit 74382a3)

bedevere-bot · 2018-04-27T19:42:07Z

GH-6620 is a backport of this pull request to the 2.7 branch.

…e. (GH-5701) (GH-6620) (cherry picked from commit 74382a3)

bpo-32857: change tkinter after_cancel to raise error if called with …

6a34b6a

…None

the-knights-who-say-ni added the CLA signed label Feb 16, 2018

bedevere-bot added the awaiting review label Feb 16, 2018

serhiy-storchaka reviewed Feb 19, 2018

View reviewed changes

Check for side effect in tests.

ebcaff9

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Feb 20, 2018

serhiy-storchaka approved these changes Feb 20, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Feb 20, 2018

csabella added 2 commits February 20, 2018 12:20

Update 2018-02-16-14-37-14.bpo-32857.-XljAx.rst

b9b9bc5

Add tests for after and after_idle

de5091b

serhiy-storchaka merged commit 74382a3 into python:master Mar 4, 2018

bedevere-bot removed the awaiting merge label Mar 4, 2018

bedevere-bot removed the needs backport to 3.7 label Mar 4, 2018

bedevere-bot removed the needs backport to 3.6 label Mar 4, 2018

miss-islington added a commit that referenced this pull request Mar 4, 2018

bpo-32857: Raise error when tkinter after_cancel() is called with Non…

73a4396

…e. (GH-5701) (cherry picked from commit 74382a3) Co-authored-by: Cheryl Sabella <[email protected]>

miss-islington added a commit that referenced this pull request Mar 4, 2018

bpo-32857: Raise error when tkinter after_cancel() is called with Non…

a5303dd

…e. (GH-5701) (cherry picked from commit 74382a3) Co-authored-by: Cheryl Sabella <[email protected]>

csabella deleted the bpo32857 branch March 4, 2018 12:54

csabella added a commit to csabella/cpython that referenced this pull request Apr 27, 2018

bpo-32857: Raise error when tkinter after_cancel() is called with Non…

aeabb7a

…e. (pythonGH-5701) (cherry picked from commit 74382a3)

bedevere-bot removed the needs backport to 2.7 label Apr 27, 2018

serhiy-storchaka pushed a commit that referenced this pull request May 5, 2018

bpo-32857: Raise error when tkinter after_cancel() is called with Non…

3a04598

…e. (GH-5701) (GH-6620) (cherry picked from commit 74382a3)

		@@ -0,0 +1 @@
		:mod:`tkinter` change handling of ``None`` parameter call to after_cancel.

Uh oh!

Conversation

csabella commented Feb 16, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Mar 4, 2018

Uh oh!

bedevere-bot commented Mar 4, 2018

Uh oh!

miss-islington commented Mar 4, 2018

Uh oh!

bedevere-bot commented Mar 4, 2018

Uh oh!

csabella commented Mar 4, 2018

Uh oh!

serhiy-storchaka commented Apr 18, 2018

Uh oh!

bedevere-bot commented Apr 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

csabella commented Feb 16, 2018 •

edited by bedevere-bot

Loading