classification
Title: IDLE: Move formatting functions into new format module.
Type: enhancement Stage: resolved
Components: IDLE Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: cheryl.sabella, taleinat, terry.reedy
Priority: normal Keywords: patch

Created on 2019-03-21 12:34 by cheryl.sabella, last changed 2019-07-18 01:23 by terry.reedy. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 12481 merged cheryl.sabella, 2019-03-21 12:38
PR 14500 merged taleinat, 2019-07-01 09:29
PR 14707 merged miss-islington, 2019-07-11 14:20
PR 14708 merged miss-islington, 2019-07-11 14:20
PR 14811 merged miss-islington, 2019-07-17 13:45
PR 14812 merged miss-islington, 2019-07-17 13:45
PR 14827 merged terry.reedy, 2019-07-17 22:53
PR 14829 merged miss-islington, 2019-07-18 00:48
PR 14830 merged miss-islington, 2019-07-18 00:48
Messages (20)
msg338537 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-03-21 12:34
In editor.py, there are several methods (indent, dedent, comment, uncomment, tabify, untabify) that are event handlers for formatting text.  To simplify testing and to simplify the EditorWindow class, refactor these methods into their own method.

This was motivated by issue36219, which is a request for another test formatting option.
msg338540 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2019-03-21 12:57
Refactoring the methods was relatively straight forward, but I did have some questions:

1.  The name `formatregion` could probably be improved.
2.  The `classifyws` method is a module level method in editor.  It's needed in `formatregion` also, so I made a copy (bad!).  I would have liked to create a `utils.py` for this, but maybe there's a better place for it?  It doesn't just operate on text widgets as it is just a string function.  I'm sure there's an obvious choice that I'm missing.  See also #4 below.
3.  `tabwidth` and `indentwidth` in `formatregion` are getting the values from the editor window.  I thought about adding those to the init for FormatRegion, but then they would need to be updated if config changed.  Not sure which way would be better.
4.  Other methods in editor might be candidates for moving, such as the auto-indent methods (search on `### begin autoindent code ###` in editor) and specifically `smart_indent_event`.  Moving more of the indent code to a separate module helps with #2 and #3 above, but I wasn't sure how much of the text formatting should be moved.  I thought it best to start with the minimal change.
msg346937 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-06-30 19:53
> 1.  The name `formatregion` could probably be improved.

FormatRegion is good enough, and is consistent with FormatParagraph.

> 2.  The `classifyws` method is a module level method in editor.  It's needed in `formatregion` also, so I made a copy (bad!).  I would have liked to create a `utils.py` for this, but maybe there's a better place for it?  It doesn't just operate on text widgets as it is just a string function.  I'm sure there's an obvious choice that I'm missing.  See also #4 below.

As you're just trying to refactor with minimal changes, this can just stay as-is IMO. 

My optimal end solution would be:
1. remove classifyws()
2. create get_line_indent(line) function which returns re.match(r'[ \t]*', line).end()
3. use len(indent) and/or len(indent.expandtabs(tabwidth)) as needed.
4. get_line_indent(line) is so simple that having it duplicated would be fine IMHO.

> 3.  `tabwidth` and `indentwidth` in `formatregion` are getting the values from the editor window.  I thought about adding those to the init for FormatRegion, but then they would need to be updated if config changed.  Not sure which way would be better.

I say, leave this the way it is.

> 4.  Other methods in editor might be candidates for moving, such as the auto-indent methods (search on `### begin autoindent code ###` in editor) and specifically `smart_indent_event`.  Moving more of the indent code to a separate module helps with #2 and #3 above, but I wasn't sure how much of the text formatting should be moved.  I thought it best to start with the minimal change.

I wouldn't move event handlers. I will say that the code in some of those functions could use a bit of cleanup and standardization WRT indentation handling, e.g. using get_line_indent() as described above in a few places where it is naively re-implemented with a loop.
msg346984 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-07-01 09:30
Looking at the code yet again, there are several places where replacing classifyws() as I suggested would make it clunkier. So let's leave it there, at least in editor.py.

Still, the classifyws() implementation can be greatly simplified. See PR GH-14500 (with tests based on those Cheryl wrote).
msg347100 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-02 03:48
I think get_indent(line) is much clearer than classifyws(s)???.  I propose to merge PR 12481 first, after review, with tests, and then include name change with rewrite in PR 14500.  Proposed docstring:
"Return line indent as # chars and effective # of spaces."
msg347101 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-02 03:55
Tabwidth for the Text widget is currently fixed at 8 based on claim in editor.py that nothing else works correctly.  It would be nice to verify if this is still correct.  If it is, then there would seem to be no need to pass it around.
msg347182 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-07-03 07:15
> I propose to merge PR 12481 first, after review, with tests, and then include name change with rewrite in PR 14500.

I think GH-14500 is in good shape now. I'd be happy to merge either PR with master after the other PR is merged, so please don't worry about the order in which we merge the PRs.
msg347681 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-07-11 14:20
New changeset 9b5ce62cac27fec9dea473865d79c2c654312957 by Tal Einat in branch 'master':
bpo-36390: simplify classifyws(), rename it and add unit tests (GH-14500)
https://github.com/python/cpython/commit/9b5ce62cac27fec9dea473865d79c2c654312957
msg347682 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-07-11 14:34
> I propose to merge PR 12481 first, after review, with tests, and then include name change with rewrite in PR 14500.

I've gone ahead and merged PR GH-14500 into master, since it was ready to get and a clear improvement.  I've already merged this into PR GH-12481.
msg347683 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-07-11 14:55
New changeset 242ad1f375bf564c35cf99cd754b2c5819c4d056 by Tal Einat (Miss Islington (bot)) in branch '3.8':
bpo-36390: simplify classifyws(), rename it and add unit tests (GH-14500)
https://github.com/python/cpython/commit/242ad1f375bf564c35cf99cd754b2c5819c4d056
msg347684 - (view) Author: Tal Einat (taleinat) * (Python committer) Date: 2019-07-11 14:58
New changeset a2cf88efc417f1991720856f6913d16660e48941 by Tal Einat (Miss Islington (bot)) in branch '3.7':
bpo-36390: simplify classifyws(), rename it and add unit tests (GH-14500)
https://github.com/python/cpython/commit/a2cf88efc417f1991720856f6913d16660e48941
msg347713 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-11 23:37
I believe I meant the opposite of what I said, which fortunately is what Tal did.  PR 14500 is the one I looked at and commented on and the name change in PR 14500 is what needed to be included in PR 12481 (done).

I would like to retitle this issue 'IDLE: Move format functions into 1 file:. and put all the functions for the Format submenu in one file, format.py, with test_format.py.  The result will be near 400 lines.  Rename paragraph.py and test_paragraph.py and put contents of proposed new files in those instead.  Add another class, FormatFile for the 3 file reforming functions.

If you want, I would not mind merging the PR as is, but with 'region' removed from the names, and adding in FormatParagraph and a new FormatFile in a separate PR.  But I don't know the consequence for the file history and git blame.  I would prefer that format.py be seen as the successor of paragraph.py if that would enable blame to trace back through the rename.

paragraph already has some utility functions, including get_indent, which is similar to get_line_indent.  We might combine them in the follow-up refactoring.  There is at least one bug, mentioned in file, to fix.

Followup refactoring should include examination of docstrings and IDLE doc.  The doc should briefly define 'region'.

SimpleDialog.AskInteger should be replaced by a Query subclass both here and in EditorWindow.goto_line_event (another issue and PR).

Menu adjustments: Move Format Paragraph to top, leaving the 3 file operations together at the bottom.  ^[ also indents.  (Tab indents if more than one line is selected.)  Strip trailing whitespace should string trailing blank lines.
msg347714 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-12 00:32
When this is merged, I will open a format menu master issue listing todos. Dependencies will include 4 issues, #24817, #5150 (two items), #18583 (with patch), and #23367 (with patch), other than #36219, follow-up code revisions, and revising README.txt.
msg348059 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-17 13:44
New changeset 82494aa6d947c4a320c09c58fe0f100cdcf7af0b by Terry Jan Reedy (Cheryl Sabella) in branch 'master':
bpo-36390: IDLE: Combine region formatting methods. (GH-12481)
https://github.com/python/cpython/commit/82494aa6d947c4a320c09c58fe0f100cdcf7af0b
msg348062 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-17 14:45
New changeset 1fc43a3fafd22eb20832459654fd125f12aa3738 by Terry Jan Reedy (Miss Islington (bot)) in branch '3.8':
bpo-36390: IDLE: Combine region formatting methods. (GH-12481) (GH-14811)
https://github.com/python/cpython/commit/1fc43a3fafd22eb20832459654fd125f12aa3738
msg348063 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-17 14:46
New changeset 093e9b1268f21624a09777818903f1088c674689 by Terry Jan Reedy (Miss Islington (bot)) in branch '3.7':
bpo-36390: IDLE: Combine region formatting methods. (GH-12481) (GH-14812)
https://github.com/python/cpython/commit/093e9b1268f21624a09777818903f1088c674689
msg348085 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-17 23:03
The new PR moves the remaining Format functions to format.py.  I only edited enough to make things work.

I did not add tests for the 2 indent methods.  They only change future indents, leaving existing indents as it.  They need change, possibly elimination, in light of 3.x's prohibition mixed tab and space indents.  They make violation way too easy.  This PR will finish this issue.
msg348090 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-18 00:48
New changeset 1b3892243433da7eae7f5f3a4f98f13d309c8926 by Terry Jan Reedy in branch 'master':
bpo-36390: Gather IDLE Format menu functions into format.py (#14827)
https://github.com/python/cpython/commit/1b3892243433da7eae7f5f3a4f98f13d309c8926
msg348091 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-18 01:21
New changeset 028f1d2479a9a508e1f0bfcff42c20c348244549 by Terry Jan Reedy (Miss Islington (bot)) in branch '3.8':
bpo-36390: Gather IDLE Format menu functions into format.py (GH-14827) (GH-14829)
https://github.com/python/cpython/commit/028f1d2479a9a508e1f0bfcff42c20c348244549
msg348092 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-07-18 01:22
New changeset 5eb19fddd2d6c70ded14a91cf083681d750d593b by Terry Jan Reedy (Miss Islington (bot)) in branch '3.7':
bpo-36390: Gather IDLE Format menu functions into format.py (GH-14827) (GH-14830)
https://github.com/python/cpython/commit/5eb19fddd2d6c70ded14a91cf083681d750d593b
History
Date User Action Args
2019-07-18 01:23:21terry.reedysetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-07-18 01:22:28terry.reedysetmessages: + msg348092
2019-07-18 01:21:11terry.reedysetmessages: + msg348091
2019-07-18 00:48:54miss-islingtonsetpull_requests: + pull_request14623
2019-07-18 00:48:48miss-islingtonsetpull_requests: + pull_request14622
2019-07-18 00:48:39terry.reedysetmessages: + msg348090
2019-07-17 23:03:52terry.reedysetmessages: + msg348085
title: IDLE: Refactor formatting methods from editor -> IDLE: Move formatting functions into new format module.
2019-07-17 22:53:01terry.reedysetpull_requests: + pull_request14621
2019-07-17 14:46:19terry.reedysetmessages: + msg348063
2019-07-17 14:45:34terry.reedysetmessages: + msg348062
2019-07-17 13:45:10miss-islingtonsetpull_requests: + pull_request14606
2019-07-17 13:45:00miss-islingtonsetpull_requests: + pull_request14605
2019-07-17 13:44:58terry.reedysetmessages: + msg348059
2019-07-12 00:32:50terry.reedysetmessages: + msg347714
2019-07-11 23:37:56terry.reedysetmessages: + msg347713
2019-07-11 14:58:10taleinatsetmessages: + msg347684
2019-07-11 14:55:26taleinatsetmessages: + msg347683
2019-07-11 14:34:17taleinatsetversions: + Python 3.7, Python 3.9
2019-07-11 14:34:03taleinatsetmessages: + msg347682
2019-07-11 14:20:51miss-islingtonsetpull_requests: + pull_request14508
2019-07-11 14:20:41miss-islingtonsetpull_requests: + pull_request14507
2019-07-11 14:20:20taleinatsetmessages: + msg347681
2019-07-03 07:15:51taleinatsetmessages: + msg347182
2019-07-02 03:55:42terry.reedysetmessages: + msg347101
2019-07-02 03:48:28terry.reedysetmessages: + msg347100
2019-07-01 09:30:21taleinatsetmessages: + msg346984
2019-07-01 09:29:02taleinatsetpull_requests: + pull_request14317
2019-06-30 19:53:23taleinatsetnosy: + taleinat
messages: + msg346937
2019-03-21 12:57:58cheryl.sabellasetmessages: + msg338540
2019-03-21 12:38:56cheryl.sabellasetkeywords: + patch
stage: patch review
pull_requests: + pull_request12433
2019-03-21 12:34:20cheryl.sabellacreate