bpo-32831: IDLE: Add docstrings and tests for codecontext #5638

csabella · 2018-02-12T13:11:44Z

https://bugs.python.org/issue32831

terryjreedy · 2018-05-17T06:00:14Z

I have updated to current upstream. Tests pass. I am reviewing and making minor changes.

terryjreedy

With the changes I made (mostly to the two longest docstrings), I think this is ready to merge. You can look at my changes first, and do a last check, but I want to merge in a day or two so this gets into 3.7.0rc1.

terryjreedy · 2018-05-17T20:34:28Z

Lib/idlelib/codecontext.py

+        editor.  It is initialized to None to not display the code context
+        and it is toggled via <<toggle-code-context>>.  If code context is
+        not suitable for an editor window, then unbind the text from
+        <<toggle-code-context>>.


I removed the part about unbinding because it happens in the editor, not here. I have, however, thought about whether binding for some features should be in the feature init.

about whether binding for some features should be in the feature init.

I tried this and it worked, but I think it should be its own issue since it's more than just code cleanup.

terryjreedy · 2018-05-17T21:56:45Z

Lib/idlelib/idle_test/test_codecontext.py

+        eq(cc.info, [(0, -1, '', False)])
+        eq(cc.topvisible, 1)
+        self.root.tk.call('after', 'info', self.cc.t1)
+        self.root.tk.call('after', 'info', self.cc.t2)


I seem to remember some discussion with Serhiy about after info, but not the outcome. Checking that the 2nd component of the tuple is 'timer' works.

The after_info PR is still open. But Serhiy had said that I wouldn't be able to use it for these tests anyway because that project couldn't be backported to 3.6. As far as that PR, I had asked him some questions on the bug tracker that he hasn't gotten to yet. I've been thinking of making the changes that I asked about just to see if it would be straight forward or not.

terryjreedy · 2018-05-17T22:09:47Z

Lib/idlelib/idle_test/test_codecontext.py

+
+    def test_del(self):
+        self.root.tk.call('after', 'info', self.cc.t1)
+        self.root.tk.call('after', 'info', self.cc.t2)


What do these do here?

Good question. Looking at the commit history, it appears that I had originally mocked the after calls and then removed the mock later. I don't remember exactly, but I think I was having trouble writing these tests because the after events still existed, even after this test suite was finished. But, once I figured it out, I think these lines could have been removed. I'm working on a patch now to address some of the other code cleanup comments and I'll remove these lines as part of that.

terryjreedy · 2018-05-17T22:17:47Z

Lib/idlelib/idle_test/test_codecontext.py

+    def setUpClass(cls):
+        requires('gui')
+        root = cls.root = Tk()
+        frame = cls.frame = Frame(root)


To minimize screen flashing, I always add root.withdraw() unless the test requires visible widgets for simulated user actions.

terryjreedy · 2018-05-17T22:28:45Z

Lib/idlelib/idle_test/test_codecontext.py

@@ -0,0 +1,345 @@
+"""Test idlelib.codecontext.
+
+Coverage: 100%


I get 99% when checking that branches are taken both ways. The condition for the following is never false.
194 elif self.topvisible > new_topvisible: # scroll up
As written, it never can be, since == and < have already been excluded.
I changed elif to else.

terryjreedy · 2018-05-17T22:34:27Z

Lib/idlelib/codecontext.py

 def getspacesfirstword(s, c=re.compile(r"^(\s*)(\w*)")):
+    "Extract the beginning whitespace and first word from s."


If we do a code cleanup pass, get_spaces_firstword and replace s with codeline.

Including this in the code cleanup PR.

terryjreedy · 2018-05-17T22:37:20Z

Lib/idlelib/codecontext.py

@@ -95,11 +125,10 @@ def toggle_code_context_event(self, event=None):
        return "break"

    def get_line_info(self, linenum):
-        """Get the line indent value, text, and any block start keyword
+        """Return tuple of (line indent value, text, and block start keyword).


If we removed self and added codeline as a parameter, this would become a helper function and testable without the cc fixture.

Including this in the code cleanup PR.

terryjreedy · 2018-05-17T22:54:33Z

Lib/idlelib/idle_test/test_codecontext.py

+    'keys': config.IdleUserConfParser(''),
+    'extensions': config.IdleUserConfParser(''),
+}
+code_sample = """


Without \ at the end, I did not initially see that the code actually starts on line 2, which made the tests more confusing. A added \ and a blank line so the sample visibly start with a blank.

terryjreedy · 2018-05-18T03:26:10Z

Given that the CI bots have become merge roadblocks, I don't want to touch this unless a change is non-trivial.

miss-islington · 2018-05-19T19:34:06Z

Thanks @csabella for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

) (cherry picked from commit 654038d) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

bedevere-bot · 2018-05-19T19:34:40Z

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

) (cherry picked from commit 654038d) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

bedevere-bot · 2018-05-19T19:35:14Z

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

(cherry picked from commit 654038d) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

csabella · 2018-05-19T21:40:43Z

@terryjreedy, thanks for the merge. Sorry I didn't get to address your questions before you committed the changes.

csabella requested a review from terryjreedy as a code owner February 12, 2018 13:11

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

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

csabella added 3 commits February 13, 2018 06:07

bpo-32831: IDLE: Add docstrings and tests for codecontext

bef38b9

Move requires(gui) before creation of root

1a50bad

Fix after tests and add coverage

fa6ebca

csabella force-pushed the codecontext branch from 12b415f to fa6ebca Compare February 13, 2018 13:17

Test for except on __del__

47ad794

terryjreedy added 2 commits May 17, 2018 15:58

Merge remote-tracking branch 'origin/master' into pr_5638

5fba5f8

Terry's changes

ae3c4ce

terryjreedy approved these changes May 17, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels May 17, 2018

terryjreedy added needs backport to 3.6 labels May 19, 2018

terryjreedy merged commit 654038d into python:master May 19, 2018

bedevere-bot removed the awaiting merge label May 19, 2018

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2018

bpo-32831: IDLE: Add docstrings and tests for codecontext (pythonGH-5638

791ebe5

) (cherry picked from commit 654038d) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

bedevere-bot removed the needs backport to 3.7 label May 19, 2018

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2018

bpo-32831: IDLE: Add docstrings and tests for codecontext (pythonGH-5638

d2f80e3

) (cherry picked from commit 654038d) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

bedevere-bot removed the needs backport to 3.6 label May 19, 2018

miss-islington added a commit that referenced this pull request May 19, 2018

bpo-32831: IDLE: Add docstrings and tests for codecontext (GH-5638)

83aedc4

(cherry picked from commit 654038d) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

miss-islington added a commit that referenced this pull request May 19, 2018

bpo-32831: IDLE: Add docstrings and tests for codecontext (GH-5638)

0efa135

(cherry picked from commit 654038d) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

csabella deleted the codecontext branch May 19, 2018 21:22

		def getspacesfirstword(s, c=re.compile(r"^(\s)(\w)")):
		"Extract the beginning whitespace and first word from s."

Uh oh!

bpo-32831: IDLE: Add docstrings and tests for codecontext #5638

bpo-32831: IDLE: Add docstrings and tests for codecontext #5638

Conversation

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

Uh oh!

Uh oh!

terryjreedy commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terryjreedy left a comment

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!

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!

terryjreedy commented May 18, 2018

Uh oh!

miss-islington commented May 19, 2018

Uh oh!

bedevere-bot commented May 19, 2018

Uh oh!

bedevere-bot commented May 19, 2018

Uh oh!

csabella commented May 19, 2018

Uh oh!

Uh oh!

csabella commented Feb 12, 2018 •

edited by bedevere-bot

Loading

terryjreedy commented May 17, 2018 •

edited

Loading