bpo-21756: fix IDLE's "show surrounding parens" for multi-line statements#20753
bpo-21756: fix IDLE's "show surrounding parens" for multi-line statements#20753taleinat wants to merge 2 commits into
Conversation
taleinat
added
needs backport to 3.7
type-bug
|
|
||
| class HyperParser: | ||
| def __init__(self, editwin, index): | ||
| def __init__(self, editwin, index, end_at_eol=True): |
There was a problem hiding this comment.
Just a nit, but I prefer
| def __init__(self, editwin, index, end_at_eol=True): | |
| def __init__(self, editwin, index, *, end_at_eol=True): |
for something like this.
There was a problem hiding this comment.
Great suggestion!
I'm still slowly chipping away at the deeply-ingrained "IDLE supports very old Python versions" rule in my mind...
There was a problem hiding this comment.
One 3.7.8 is out, we can consider using new-in-3.8 features such as '/' in signatures and ':='.
| if r: | ||
| stopatindex = text.index(r[0] + "-1c") | ||
| else: | ||
| stopatindex = "end-1c" |
There was a problem hiding this comment.
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"
|
Nice! This works well. Also #18539 changes this code a little. It doesn't fix this issue, but we'll need to rebase whichever one is merged second. |
|
|
||
| class HyperParser: | ||
| def __init__(self, editwin, index): | ||
| def __init__(self, editwin, index, end_at_eol=True): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
endline, multiline, oneline, singleline, continuation?
| startat = max(lno - context, 1) | ||
| startatindex = repr(startat) + ".0" | ||
| stopatindex = "%d.end" % lno | ||
| stopatindex = "%d.end" % lno if end_at_eol else "end-1c" |
There was a problem hiding this comment.
Since you're changing this line, an f-string would make this more readable.
|
Thanks for taking a look at this @csabella! I've made some changes based on your suggestions. It would be great if you could take another look :) |
|
I also added a test and then discovered that this breaks rather badly in some cases... The underlying PyParse parser strongly assumes that it's only given code up to the end of the current statement. |
|
An possible alternative is to leave |
Always! :-) |
|
So, as it stands, this PR doesn't work properly, and breaks certain use-cases :( It's not a minor problem with the PR, it a problem with the approach. So I'm closing this PR. Properly fixing bpo-21756 will simply require a more significant refactoring. Specifically, it will likely need a version of the code parser which can handle lines incrementally, maybe both backwards (to find the beginning of a code block) and forwards (to find the end). |
5 participants
https://bugs.python.org/issue21756