IDLE: Move formatting functions into new format module.

[csabella]

BPO	36390
Nosy	@terryjreedy, @taleinat, @csabella
PRs	bpo-36390: IDLE: Combine region formatting methods. #12481 bpo-36390: simplify classifyws() and add unit tests #14500 [3.8] bpo-36390: simplify classifyws(), rename it and add unit tests (GH-14500) #14707 [3.7] bpo-36390: simplify classifyws(), rename it and add unit tests (GH-14500) #14708 [3.8] bpo-36390: IDLE: Combine region formatting methods. (GH-12481) #14811 [3.7] bpo-36390: IDLE: Combine region formatting methods. (GH-12481) #14812 bpo-36390: Gather IDLE Format menu functions into format.py #14827 [3.8] bpo-36390: Gather IDLE Format menu functions into format.py (GH-14827) #14829 [3.7] bpo-36390: Gather IDLE Format menu functions into format.py (GH-14827) #14830

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/terryjreedy'
closed_at = <Date 2019-07-18.01:23:21.044>
created_at = <Date 2019-03-21.12:34:20.915>
labels = ['3.8', 'expert-IDLE', 'type-feature', '3.7', '3.9']
title = 'IDLE: Move formatting functions into new format module.'
updated_at = <Date 2019-07-18.01:23:21.044>
user = 'https://github.com/csabella'

bugs.python.org fields:

activity = <Date 2019-07-18.01:23:21.044>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2019-07-18.01:23:21.044>
closer = 'terry.reedy'
components = ['IDLE']
creation = <Date 2019-03-21.12:34:20.915>
creator = 'cheryl.sabella'
dependencies = []
files = []
hgrepos = []
issue_num = 36390
keywords = ['patch']
message_count = 20.0
messages = ['338537', '338540', '346937', '346984', '347100', '347101', '347182', '347681', '347682', '347683', '347684', '347713', '347714', '348059', '348062', '348063', '348085', '348090', '348091', '348092']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'taleinat', 'cheryl.sabella']
pr_nums = ['12481', '14500', '14707', '14708', '14811', '14812', '14827', '14829', '14830']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue36390'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

[csabella]

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 bpo-36219, which is a request for another test formatting option.

[csabella]

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 Update Python Software Foundation Copyright Year. #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 Rename README to README.rst and enhance formatting #2 and bpo-29403: Fix mock's broken autospec behavior on method-bound builtin functions #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.

[taleinat]

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 Update Python Software Foundation Copyright Year. #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 Rename README to README.rst and enhance formatting #2 and bpo-29403: Fix mock's broken autospec behavior on method-bound builtin functions #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.

[taleinat]

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 #58705 (with tests based on those Cheryl wrote).

[terryjreedy]

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."

[terryjreedy]

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.

[taleinat]

I propose to merge PR 12481 first, after review, with tests, and then include name change with rewrite in PR 14500.

I think #58705 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.

[taleinat]

New changeset 9b5ce62 by Tal Einat in branch 'master':
bpo-36390: simplify classifyws(), rename it and add unit tests (GH-14500)
9b5ce62

[taleinat]

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 #58705 into master, since it was ready to get and a clear improvement. I've already merged this into PR #56690.