bpo-36390: IDLE: Combine region formatting methods. #12481

csabella · 2019-03-21T12:38:55Z

Create module for indent, dedent, comment, uncomment, tabify, and untabify.
Add tests for new module.

https://bugs.python.org/issue36390

* Create module for indent, dedent, comment, uncomment, tabify, and untabify. * Add tests for new module.

taleinat · 2019-06-06T18:14:16Z

Lib/idlelib/formatregion.py

+        ## is appended to the beginning of each line to comment it out.
+        """
+        head, tail, chars, lines = self.get_region()
+        for pos in range(len(lines) - 1):


Why is the last line excluded?

If the last line ends in a newline, then get_region() will return an empty string as the last value. For example, the the text is a = 'hello'\n, then lines contains ['a = hello', '']. Again, I didn't alter the existing code from editor.py, so if this needs to be improved, I think it should be in a subsequent PR.

I agree with leave as is now. By manual test, the code works (almost*) correctly. Note 1. 'a\n'.splitlines() == ['a'], 'a\n'.split('\n') == ['a', '']; get_region does the latter. *Perhaps' it should do the former. *If cursor is at the end of a file, (such as when opening a new file), a new newline may be added. Adding trailing blank lines is a bug.

taleinat · 2019-06-06T18:15:04Z

Lib/idlelib/formatregion.py

+        text.undo_block_stop()
+        text.tag_add("sel", head, "insert")
+
+    def indent_region_event(self, event=None):


This and the other methods below have so much in common that they should probably be refactored.

I agree, but I wanted to make this PR about removing them from editor.py, so I kept them as they were currently written. Once the code has been split from editor.py and the tests are in place, then I'll work on refactoring.

I have noticed duplication also, and I agree with leaving them as-is until a follow-up.

taleinat · 2019-06-06T18:20:07Z

@csabella, excellent job adding the tests for all of this functionality!

csabella · 2019-06-30T15:02:15Z

Hi @taleinat, thank you for the review. I had tried to make the minimal changes in this PR to just extract the code from editor.py. It is challenging to not make more changes at the same time, but I really wanted to try to keep it as simple as possible for review. Once the formatting functions are split, it will be easier to focus on improving just these methods and also adding new formatting methods to the menu.

Lib/idlelib/idle_test/test_formatregion.py

taleinat · 2019-06-30T20:03:30Z

Lib/idlelib/idle_test/test_formatregion.py

+
+        # Remove selection.
+        text.tag_remove('sel', '1.0', 'end')
+        eq(get(), ('15.0', '16.0', '\n', ['', '']))


This is very surprising: Why is this returned? This should have a comment, and perhaps explicitly set the insertion index before calling get_region().

The sample has 14 lines, all with newline. Apparently, tag_remove to 'end' puts the cursor after the last newline, which is 15,0. Yes, say so. To get the tail index, get_region adds 1c, resulting in 16,0, after the hidden newline tk maintains as the end of every file. It then split('\n')s on that hidden newline, giving the fake list. set_region may (wrongly) add a new newline, as I said above.

taleinat · 2019-06-30T20:05:57Z

Lib/idlelib/idle_test/test_formatregion.py

+        self.text.delete('1.0', 'end')
+
+    def test_get_region(self):
+        get = self.formatter.get_region


I find all of these assignments make the tests harder to read rather than easier. It increases the number of things I need to remember, and makes it necessary to read every test method from the beginning to understand any part of it.

This is a matter of style, but IMO when using many short test methods as done here (which I like!) these "shortcuts" do more harm than good.

To be clear, this is true for all of the test methods here.

Cheryl learned this from me ;-). I get bleary-eyed reading self.assertEqual too many times. Aside from that, the short func name, usually followed by 'eq' are local conventions that we understand immediately, and you don't -- yet. The 'text' abbreviation in not needed for 2 uses and an be deleted and expanded back.

Looking more, all functions start with func =, text =, eq =, and I think uniformity is better than variety here.

taleinat · 2019-06-30T20:09:22Z

Lib/idlelib/idle_test/test_formatregion.py

+        del cls.root
+
+    def setUp(self):
+        self.text.insert('1.0', code_sample)


Using a single code sample makes the tests harder to read IMO. Worse, in the future, it will make maintenance difficult: Any change to the code sample would require updating many of the tests.

I suggest having each test define its own code sample(s), which will make them more self-contained.

Tal, this and the next comment suggest that your eyes and brain and mine work a bit differently, such that I appear to be more bothered by redundancy than your are.

In this case, I agree that a) the sample is too far from from code that depends on its fine details, and b) 1 sample is likely too few, perhaps more complicated than needed for any of the subset of tests. Perhaps one for the 2 region functions, just before test_get_region, one for the 4 indent/comment function, just before the first of those, and one for the 2 tab functions.

Sample moved in commit 'Move code sample.'. I want to mostly leave tests alone for now because we may refactor them along with the functions they test.

Lib/idlelib/idle_test/test_formatregion.py

taleinat · 2019-06-30T20:14:48Z

Lib/idlelib/idle_test/test_formatregion.py

+        # Add selection.
+        text.tag_add('sel', '7.0', '10.0')
+        eq(get(), ('7.0', '10.0',
+                   '\n    def compare(self):\n        if a > b:\n',


Could this be re-written to use a multi-line string? I think it would be clearer. Or better, avoid duplication entirely, e.g.:

expected_lines = [ '', ' def compare(self):', ' if a > b:', '', ] eq(get(), ('7.0', '10.0', '\n'.join(expected_lines), expected_lines))

I very much agree with removing this redundancy. Given no shortage of horizontal space, I would likely write it without the newlines after [ and before ].

expected_lines = ['', ' def compare(self):', ' if a > b:', '',]

# Conflicts: # Lib/idlelib/editor.py

terryjreedy

My unposted todo for Format is "Consolidate into 1 file. Paragraph, Rstrip, stuff in editor." My first concern is that this issue and PR be at least a step towards doing that, as discussed on the issue.

My second goal was responding to Tal's comments.

terryjreedy · 2019-07-11T20:09:08Z

Lib/idlelib/formatregion.py

+        text.undo_block_stop()
+        text.tag_add("sel", head, "insert")
+
+    def indent_region_event(self, event=None):


I have noticed duplication also, and I agree with leaving them as-is until a follow-up.

Lib/idlelib/formatregion.py

terryjreedy · 2019-07-11T21:49:35Z

Lib/idlelib/formatregion.py

+        ## is appended to the beginning of each line to comment it out.
+        """
+        head, tail, chars, lines = self.get_region()
+        for pos in range(len(lines) - 1):


I agree with leave as is now. By manual test, the code works (almost*) correctly. Note 1. 'a\n'.splitlines() == ['a'], 'a\n'.split('\n') == ['a', '']; get_region does the latter. *Perhaps' it should do the former. *If cursor is at the end of a file, (such as when opening a new file), a new newline may be added. Adding trailing blank lines is a bug.

terryjreedy · 2019-07-11T23:09:32Z

Lib/idlelib/idle_test/test_formatregion.py

+        del cls.root
+
+    def setUp(self):
+        self.text.insert('1.0', code_sample)


Tal, this and the next comment suggest that your eyes and brain and mine work a bit differently, such that I appear to be more bothered by redundancy than your are.

In this case, I agree that a) the sample is too far from from code that depends on its fine details, and b) 1 sample is likely too few, perhaps more complicated than needed for any of the subset of tests. Perhaps one for the 2 region functions, just before test_get_region, one for the 4 indent/comment function, just before the first of those, and one for the 2 tab functions.

terryjreedy · 2019-07-11T23:21:04Z

Lib/idlelib/idle_test/test_formatregion.py

+        self.text.delete('1.0', 'end')
+
+    def test_get_region(self):
+        get = self.formatter.get_region


Cheryl learned this from me ;-). I get bleary-eyed reading self.assertEqual too many times. Aside from that, the short func name, usually followed by 'eq' are local conventions that we understand immediately, and you don't -- yet. The 'text' abbreviation in not needed for 2 uses and an be deleted and expanded back.

terryjreedy · 2019-07-11T23:21:32Z

Lib/idlelib/idle_test/test_formatregion.py

+        # Add selection.
+        text.tag_add('sel', '7.0', '10.0')
+        eq(get(), ('7.0', '10.0',
+                   '\n    def compare(self):\n        if a > b:\n',


I very much agree with removing this redundancy. Given no shortage of horizontal space, I would likely write it without the newlines after [ and before ].

expected_lines = ['', ' def compare(self):', ' if a > b:', '',]

terryjreedy · 2019-07-11T23:36:37Z

Lib/idlelib/idle_test/test_formatregion.py

+
+        # Remove selection.
+        text.tag_remove('sel', '1.0', 'end')
+        eq(get(), ('15.0', '16.0', '\n', ['', '']))


The sample has 14 lines, all with newline. Apparently, tag_remove to 'end' puts the cursor after the last newline, which is 15,0. Yes, say so. To get the tail index, get_region adds 1c, resulting in 16,0, after the hidden newline tk maintains as the end of every file. It then split('\n')s on that hidden newline, giving the fake list. set_region may (wrongly) add a new newline, as I said above.

bedevere-bot · 2019-07-12T00:24:39Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

terryjreedy · 2019-07-16T05:46:13Z

Cheryl, you said elsewhere that you cannot work on issues for a bit. I would, then, like to finish this myself so I can move on to followups.

terryjreedy · 2019-07-17T01:35:36Z

I am working on this now.

Make other changes so manual testing passes.

Edit to make tests pass.

csabella · 2019-07-17T06:24:43Z

Terry, thank you for working on this. I had hoped to get some bandwidth after this past weekend, but it doesn't look like that's going to happen, so I appreciate you moving this forward. And Tal, thank you for the review and apologies for not getting to it.

taleinat · 2019-07-17T08:39:55Z

@csabella, I reviewed hoping to help this along, but please don't feel pressured by that! Take your time and do things whenever you can find the time for them. There's certainly no need to apologize!

miss-islington · 2019-07-17T13:44:47Z

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

Rename paragraph.py to format.py and add region formatting methods from editor.py. Add tests for the latter. (cherry picked from commit 82494aa) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

bedevere-bot · 2019-07-17T13:45:01Z

GH-14811 is a backport of this pull request to the 3.8 branch.

Rename paragraph.py to format.py and add region formatting methods from editor.py. Add tests for the latter. (cherry picked from commit 82494aa) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

bedevere-bot · 2019-07-17T13:45:12Z

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

) Rename paragraph.py to format.py and add region formatting methods from editor.py. Add tests for the latter. (cherry picked from commit 82494aa) Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>

Rename paragraph.py to format.py and add region formatting methods from editor.py. Add tests for the latter.

bpo-36390: IDLE: Refactor formatting methods from editor.py.

d07fa50

* Create module for indent, dedent, comment, uncomment, tabify, and untabify. * Add tests for new module.

csabella requested a review from terryjreedy as a code owner March 21, 2019 12:38

the-knights-who-say-ni added the CLA signed label Mar 21, 2019

bedevere-bot added the awaiting merge label Mar 21, 2019

taleinat reviewed Jun 6, 2019

View reviewed changes

taleinat reviewed Jun 30, 2019

View reviewed changes

Merge branch 'master' into region

3f355ab

# Conflicts: # Lib/idlelib/editor.py

terryjreedy requested changes Jul 12, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jul 12, 2019

terryjreedy added 6 commits July 16, 2019 21:37

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

1f09b60

Consolidate paragraph and formatregion into format.

7056507

Make other changes so manual testing passes.

Combine test_paragraph and test_formatregion into test_format.

bf4fa44

Edit to make tests pass.

remove test_formatregion

6eeb1e9

Move code sample.

7df3f49

Edit expected lines.

f97af53

terryjreedy changed the title ~~bpo-36390: IDLE: Refactor formatting methods from editor.py.~~ bpo-36390: IDLE: Combine region formatting methods. Jul 17, 2019

Edit blurb and coverage.

fb87fc1

terryjreedy approved these changes Jul 17, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jul 17, 2019

terryjreedy added needs backport to 3.7 needs backport to 3.8 only security fixes type-feature A feature request or enhancement labels Jul 17, 2019

terryjreedy added the tests Tests in the Lib/test dir label Jul 17, 2019

terryjreedy merged commit 82494aa into python:master Jul 17, 2019

bedevere-bot removed the awaiting merge label Jul 17, 2019

bedevere-bot removed the needs backport to 3.8 only security fixes label Jul 17, 2019

bedevere-bot removed the needs backport to 3.7 label Jul 17, 2019

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019

bpo-36390: IDLE: Combine region formatting methods. (pythonGH-12481)

56aa74b

Rename paragraph.py to format.py and add region formatting methods from editor.py. Add tests for the latter.

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020

bpo-36390: IDLE: Combine region formatting methods. (pythonGH-12481)

43097e9

Rename paragraph.py to format.py and add region formatting methods from editor.py. Add tests for the latter.

csabella deleted the region branch February 23, 2020 00:33

websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020

bpo-36390: IDLE: Combine region formatting methods. (pythonGH-12481)

d9343cf

Rename paragraph.py to format.py and add region formatting methods from editor.py. Add tests for the latter.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-36390: IDLE: Combine region formatting methods. #12481

bpo-36390: IDLE: Combine region formatting methods. #12481

csabella commented Mar 21, 2019 •

edited by bedevere-bot

taleinat Jun 6, 2019

csabella Jun 30, 2019

terryjreedy Jul 11, 2019

taleinat Jun 6, 2019

csabella Jun 30, 2019

terryjreedy Jul 11, 2019

taleinat commented Jun 6, 2019

csabella commented Jun 30, 2019

taleinat Jun 30, 2019

terryjreedy Jul 11, 2019

taleinat Jun 30, 2019

terryjreedy Jul 11, 2019

terryjreedy Jul 17, 2019

taleinat Jun 30, 2019 •

edited

terryjreedy Jul 11, 2019

terryjreedy Jul 17, 2019 •

edited

taleinat Jun 30, 2019

terryjreedy Jul 11, 2019

terryjreedy left a comment

terryjreedy Jul 11, 2019

terryjreedy Jul 11, 2019

terryjreedy Jul 11, 2019

terryjreedy Jul 11, 2019

terryjreedy Jul 11, 2019

terryjreedy Jul 11, 2019

bedevere-bot commented Jul 12, 2019

terryjreedy commented Jul 16, 2019

terryjreedy commented Jul 17, 2019

csabella commented Jul 17, 2019

taleinat commented Jul 17, 2019

miss-islington commented Jul 17, 2019

bedevere-bot commented Jul 17, 2019

bedevere-bot commented Jul 17, 2019

bpo-36390: IDLE: Combine region formatting methods. #12481

bpo-36390: IDLE: Combine region formatting methods. #12481

Conversation

csabella commented Mar 21, 2019 • edited by bedevere-bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taleinat commented Jun 6, 2019

csabella commented Jun 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taleinat Jun 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy Jul 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 12, 2019

terryjreedy commented Jul 16, 2019

terryjreedy commented Jul 17, 2019

csabella commented Jul 17, 2019

taleinat commented Jul 17, 2019

miss-islington commented Jul 17, 2019

bedevere-bot commented Jul 17, 2019

bedevere-bot commented Jul 17, 2019

csabella commented Mar 21, 2019 •

edited by bedevere-bot

taleinat Jun 30, 2019 •

edited

terryjreedy Jul 17, 2019 •

edited