bpo-32831: IDLE: Add docstrings and tests for codecontext#5638
Conversation
12b415f to
fa6ebca
Compare
|
I have updated to current upstream. Tests pass. I am reviewing and making minor changes. |
terryjreedy
left a comment
There was a problem hiding this comment.
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.
| 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>>. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| def test_del(self): | ||
| self.root.tk.call('after', 'info', self.cc.t1) | ||
| self.root.tk.call('after', 'info', self.cc.t2) |
There was a problem hiding this comment.
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.
| def setUpClass(cls): | ||
| requires('gui') | ||
| root = cls.root = Tk() | ||
| frame = cls.frame = Frame(root) |
There was a problem hiding this comment.
To minimize screen flashing, I always add root.withdraw() unless the test requires visible widgets for simulated user actions.
| @@ -0,0 +1,345 @@ | |||
| """Test idlelib.codecontext. | |||
|
|
|||
| Coverage: 100% | |||
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| def getspacesfirstword(s, c=re.compile(r"^(\s*)(\w*)")): | ||
| "Extract the beginning whitespace and first word from s." |
There was a problem hiding this comment.
If we do a code cleanup pass, get_spaces_firstword and replace s with codeline.
There was a problem hiding this comment.
Including this in the code cleanup PR.
|
|
||
| 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). |
There was a problem hiding this comment.
If we removed self and added codeline as a parameter, this would become a helper function and testable without the cc fixture.
There was a problem hiding this comment.
Including this in the code cleanup PR.
| 'keys': config.IdleUserConfParser(''), | ||
| 'extensions': config.IdleUserConfParser(''), | ||
| } | ||
| code_sample = """ |
There was a problem hiding this comment.
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.
|
Given that the CI bots have become merge roadblocks, I don't want to touch this unless a change is non-trivial. |
|
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>
|
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>
|
GH-6989 is a backport of this pull request to the 3.6 branch. |
|
@terryjreedy, thanks for the merge. Sorry I didn't get to address your questions before you committed the changes. |
https://bugs.python.org/issue32831