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.

Sign up for GitHub

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

Jump to bottom

bpo-21756: fix IDLE's "show surrounding parens" for multi-line statements #20753

Closed
wants to merge 2 commits into from
Closed

bpo-21756: fix IDLE's "show surrounding parens" for multi-line statements #20753

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f1700d
bpo-21756: fix IDLE's "show surrounding parens" for multi-line statem…
taleinat Jun 9, 2020
843d18d
refactoring based on csabella's CR
taleinat Jun 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 14 additions & 6 deletions Lib/idlelib/hyperparser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@


class HyperParser:
def __init__(self, editwin, index):
def __init__(self, editwin, index, end_at_eol=True):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit, but I prefer

Suggested change
def __init__(self, editwin, index, end_at_eol=True):
def __init__(self, editwin, index, *, end_at_eol=True):

for something like this.

Copy link
Contributor Author

@taleinat taleinat Jun 10, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion!

I'm still slowly chipping away at the deeply-ingrained "IDLE supports very old Python versions" rule in my mind...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One 3.7.8 is out, we can consider using new-in-3.8 features such as '/' in signatures and ':='.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think wrap or something like it would work instead of end_at_eol? I had to stop and think about end_at_eol to comprehend its meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the name "end_at_eol" isn't great. I'm not sure "wrap" is clear enough though. Perhaps "stop_at_line_end" or "multiline"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer one word or at most two (which don't necessarily need '_') rather than a phrase. The docstring can explain the word. In any case, the audience is only IDLE maintainers and there may never be more than one or two calls that change the current default. 'endline=True' is a possibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So "multiline"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

endline, multiline, oneline, singleline, continuation?

"To initialize, analyze the surroundings of the given index."

self.editwin = editwin
Expand All @@ -39,7 +39,7 @@ def index2line(index):
for context in editwin.num_context_lines:
startat = max(lno - context, 1)
startatindex = repr(startat) + ".0"
stopatindex = "%d.end" % lno
stopatindex = "%d.end" % lno if end_at_eol else "end-1c"
Copy link
Contributor

@csabella csabella Jun 9, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're changing this line, an f-string would make this more readable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely.

# We add the newline because PyParse requires a newline
# at end. We add a space so that index won't be at end
# of line, so that its status will be the same as the
Expand All @@ -56,7 +56,14 @@ def index2line(index):
startatindex = r[1]
else:
startatindex = "1.0"
stopatindex = "%d.end" % lno
if end_at_eol:
stopatindex = "%d.end" % lno
else:
r = text.tag_nextrange("console", index)
if r:
stopatindex = text.index(r[0] + "-1c")
else:
stopatindex = "end-1c"
Copy link
Contributor

@csabella csabella Jun 9, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else is a bit unwieldy. Since hyperparser has tests, maybe it can be restructured with a high confidence of correctness?

        else:
            r = text.tag_prevrange("console", index)
            if r:
                startatindex = r[1]
            else:
                startatindex = "1.0"
            stopatindex = "%d.end" % lno
            if end_at_eol:
                stopatindex = "%d.end" % lno
            else:
                r = text.tag_nextrange("console", index)
                if r:
                    stopatindex = text.index(r[0] + "-1c")
                else:
                    stopatindex = "end-1c"

# We add the newline because PyParse requires it. We add a
# space so that index won't be at end of line, so that its
# status will be the same as the char before it, if should.
Expand Down Expand Up @@ -119,10 +126,11 @@ def get_surrounding_brackets(self, openers='([{', mustclose=False):
If the index given to the HyperParser is surrounded by a
bracket defined in openers (or at least has one before it),
return the indices of the opening bracket and the closing
bracket (or the end of line, whichever comes first).
bracket (or the end of line/file, whichever comes first).

If it is not surrounded by brackets, or the end of line comes
before the closing bracket and mustclose is True, returns None.
If it is not surrounded by brackets, or the end of line/file
comes before the closing bracket and mustclose is True, returns
None.
"""

bracketinglevel = self.bracketing[self.indexbracket][1]
Expand Down
4 changes: 2 additions & 2 deletions Lib/idlelib/parenmatch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ def deactivate_restore(self):

def flash_paren_event(self, event):
"Handle editor 'show surrounding parens' event (menu or shortcut)."
indices = (HyperParser(self.editwin, "insert")
.get_surrounding_brackets())
hp = HyperParser(self.editwin, "insert", end_at_eol=False)
indices = hp.get_surrounding_brackets()
self.finish_paren_event(indices)
return "break"

Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/IDLE/2020-06-09-10-47-33.bpo-21756.auszVR.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed IDLE's "show surrounding parens" for multi-line statements.