Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDLE: Move formatting functions into new format module. #80571

Closed
csabella opened this issue Mar 21, 2019 · 20 comments
Closed

IDLE: Move formatting functions into new format module. #80571

csabella opened this issue Mar 21, 2019 · 20 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes topic-IDLE type-feature A feature request or enhancement

Comments

@csabella
Copy link
Contributor

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
    Copy link
    Contributor Author

    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 csabella added the 3.8 only security fixes label Mar 21, 2019
    @csabella csabella added topic-IDLE type-feature A feature request or enhancement labels Mar 21, 2019
    @csabella
    Copy link
    Contributor Author

    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
    Copy link
    Contributor

    1. The name formatregion could probably be improved.

    FormatRegion is good enough, and is consistent with FormatParagraph.

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

    1. 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
    Copy link
    Contributor

    taleinat commented Jul 1, 2019

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Contributor

    taleinat commented Jul 3, 2019

    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
    Copy link
    Contributor

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

    @taleinat
    Copy link
    Contributor

    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.

    @taleinat taleinat added 3.7 (EOL) end of life 3.9 only security fixes labels Jul 11, 2019
    @taleinat
    Copy link
    Contributor

    New changeset 242ad1f by Tal Einat (Miss Islington (bot)) in branch '3.8':
    bpo-36390: simplify classifyws(), rename it and add unit tests (GH-14500)
    242ad1f

    @taleinat
    Copy link
    Contributor

    New changeset a2cf88e by Tal Einat (Miss Islington (bot)) in branch '3.7':
    bpo-36390: simplify classifyws(), rename it and add unit tests (GH-14500)
    a2cf88e

    @terryjreedy
    Copy link
    Member

    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.

    @terryjreedy
    Copy link
    Member

    When this is merged, I will open a format menu master issue listing todos. Dependencies will include 4 issues, bpo-24817, bpo-5150 (two items), bpo-18583 (with patch), and bpo-23367 (with patch), other than bpo-36219, follow-up code revisions, and revising README.txt.

    @terryjreedy
    Copy link
    Member

    New changeset 82494aa by Terry Jan Reedy (Cheryl Sabella) in branch 'master':
    bpo-36390: IDLE: Combine region formatting methods. (GH-12481)
    82494aa

    @terryjreedy
    Copy link
    Member

    New changeset 1fc43a3 by Terry Jan Reedy (Miss Islington (bot)) in branch '3.8':
    bpo-36390: IDLE: Combine region formatting methods. (GH-12481) (GH-14811)
    1fc43a3

    @terryjreedy
    Copy link
    Member

    New changeset 093e9b1 by Terry Jan Reedy (Miss Islington (bot)) in branch '3.7':
    bpo-36390: IDLE: Combine region formatting methods. (GH-12481) (GH-14812)
    093e9b1

    @terryjreedy
    Copy link
    Member

    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.

    @terryjreedy terryjreedy changed the title IDLE: Refactor formatting methods from editor IDLE: Move formatting functions into new format module. Jul 17, 2019
    @terryjreedy
    Copy link
    Member

    New changeset 1b38922 by Terry Jan Reedy in branch 'master':
    bpo-36390: Gather IDLE Format menu functions into format.py (bpo-14827)
    1b38922

    @terryjreedy
    Copy link
    Member

    New changeset 028f1d2 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)
    028f1d2

    @terryjreedy
    Copy link
    Member

    New changeset 5eb19fd 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)
    5eb19fd

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes topic-IDLE type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants