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

bpo-44010: IDLE: colorize pattern-matching soft keywords #25851

Merged
merged 31 commits into from
May 19, 2021

Conversation

taleinat
Copy link
Contributor

@taleinat taleinat commented May 3, 2021

This implements highlighting of the match, case and _ soft keywords, following the syntax defined in the docs to the extent reasonably possible within the framework of the current colorization method.

https://bugs.python.org/issue44010

Lib/idlelib/colorizer.py Outdated Show resolved Hide resolved
Lib/idlelib/colorizer.py Outdated Show resolved Hide resolved
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

Code review only so far.

Lib/idlelib/colorizer.py Outdated Show resolved Hide resolved
Lib/idlelib/colorizer.py Outdated Show resolved Hide resolved
Lib/idlelib/colorizer.py Outdated Show resolved Hide resolved
Lib/idlelib/colorizer.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@terryjreedy
Copy link
Member

I now agree that this is much more important than f-string field expressions. Highlighting helps one read complicated code and also to understand some errors. Field expressions tend to be short and simple. Most would get no markup other than 'not string'.

@python python deleted a comment May 4, 2021
@python python deleted a comment May 4, 2021
@taleinat
Copy link
Contributor Author

taleinat commented May 6, 2021

I've refactored the code somewhat and made it mark all lone underscores in case <...>: as keywords (after going down several wrong roads...). I think it'll need more tweaking to handle guards, though.

@taleinat
Copy link
Contributor Author

taleinat commented May 6, 2021

I still haven't done away with the duplicated test code fragment, but I'll make sure to do so if we move forward with this.

@terryjreedy
Copy link
Member

'_' is only and always a keyword in patterns, but not in guards or anywhere else. Simple rule, fortunately. The guards are standard python, as are the actions and match line. Doc has been patched to say do. The example from one of the outdated comments 2 day ago:

>>> _ = 'a'
>>> match _:
...     case _ if _: print(_, 'ok')
...  
a ok

@E-Paine
Copy link
Contributor

E-Paine commented May 6, 2021

I am not sure this quite works, sadly. IMO, we need to recursively colour all content in the guard:

case _ if ("a" if _ else set()): pass

@taleinat
Copy link
Contributor Author

taleinat commented May 6, 2021

'_' is only and always a keyword in patterns, but not in guards or anywhere else.

A bare _ is also explicitly, specifically disallowed in pattern-matching "capture groups", i.e. case <pattern> as _: is disallowed.

The latest commits do mark a bare _ as a keyword in this case, which I think we agree is the desired behavior.

@taleinat
Copy link
Contributor Author

taleinat commented May 6, 2021

I am not sure this quite works, sadly. IMO, we need to recursively colour all content in the guard:

case _ if ("a" if _ else set()): pass

Fixed, and I've added that example to the tests.

@taleinat taleinat requested a review from terryjreedy May 6, 2021 21:35
@taleinat
Copy link
Contributor Author

@terryjreedy, I've made the doc changes you suggested.

I've also updated help.html. Note that it will require manual backporting to continue showing correct version numbers, so I'm removing the auto-backport label.

@taleinat
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from terryjreedy May 11, 2021 07:58
@taleinat taleinat removed the needs backport to 3.10 only security fixes label May 11, 2021
@terryjreedy terryjreedy added the needs backport to 3.10 only security fixes label May 11, 2021
@terryjreedy
Copy link
Member

The version numbers and other extraneous material in help.html are outside of the part of the file displayed, which is <div class='section>, lines 103 to 878. They have always been the same in all active versions so that there is never an issue with backporting.

This is the first time we have not backported a doc change to all versions. So if we do nothing, there would be merge conflicts the next time we backport a doc change to 3.9. To avoid this, help.html should be copied, after merging, to the 3.9 branch. The new paragraph, lines 584 to 588, should be deleted. The change on 693, which should have been incorporated into the file previously, and all other changes, should be merged into 3.9.

I have considered moving the 'section' extraction mechanism from display to copying. The result would have to be tested and might be less robust against sphinx formatting changes, but the diffs and the code would be simpler.

@taleinat
Copy link
Contributor Author

@terryjreedy, do you want anything else done here? Are you okay with this being merged in its current form?

To avoid this, help.html should be copied, after merging, to the 3.9 branch. The new paragraph, lines 584 to 588, should be deleted. The change on 693, which should have been incorporated into the file previously, and all other changes, should be merged into 3.9.

Alright, we can take care of that in a followup PR.

@terryjreedy
Copy link
Member

News entries look fine, so go ahead. I will handle idlelib/NEWS separately. Also 3.9 doc patch if you don't.

@taleinat taleinat merged commit 60d343a into python:main May 19, 2021
@miss-islington
Copy link
Contributor

Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@taleinat taleinat deleted the idle-colorize-soft-keywords branch May 19, 2021 09:18
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2021
@bedevere-bot
Copy link

GH-26237 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label May 19, 2021
miss-islington added a commit that referenced this pull request May 19, 2021
@taleinat
Copy link
Contributor Author

Alright, we can take care of that in a followup PR.

See PR GH-26239.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants