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

GH-83162: Rename re.error for better clarity. #101677

Merged
merged 20 commits into from
Dec 11, 2023
Merged

GH-83162: Rename re.error for better clarity. #101677

merged 20 commits into from
Dec 11, 2023

Conversation

achhina
Copy link
Contributor

@achhina achhina commented Feb 8, 2023 •
edited
Loading

Follow-up of rebased/squashed PR-1751 - renaming re.error for better clarity.

TODO

  • Settle on new exception name. We seem to have a consensus for PatternError.
  • Follow-up on whether we rename the exception in idlelib/replace.py. Approved for now.
  • Ask whether this should also be added Python 3.13 What's New. Added to Python 3.13 What's New.

@bedevere-bot bedevere-bot mentioned this pull request Feb 8, 2023
Closed
@barneygale
Copy link
Contributor

Maybe it's just me, but I initially read ReCompileError as "recompile error", despite the capitalisation of the 'C'. Would PatternCompileError or just PatternError be any better?

@achhina
Copy link
Contributor Author

achhina commented Feb 9, 2023

Maybe it's just me, but I initially read ReCompileError as "recompile error", despite the capitalisation of the 'C'. Would PatternCompileError or just PatternError be any better?

Thanks for pointing that out, I agree that it can easily be read as recompile error. I think both PatternCompileError & PatternError would also work well, but I prefer RESyntaxError for some of the reasons I laid out in the original issue thread - where we should consolidate this discussion. However, happy to hear otherwise!

@SylvainDe
Copy link
Contributor

Nice PR!
I'm not sure if this comment should be on the issue or on the PR but could we make the error name easier to read by using the ""full"" name "Regex" instead of "re".

hugovk
hugovk reviewed May 23, 2023
View reviewed changes
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

  • Ask whether this should also be added Python 3.12 What's New

Yes please, in the 3.13 one.

Doc/library/re.rst Outdated Show resolved Hide resolved
Lib/re/_constants.py Outdated Show resolved Hide resolved
@hugovk hugovk added the 3.13 bugs and security fixes label May 23, 2023
@achhina
Copy link
Contributor Author

achhina commented May 29, 2023

Nice PR! I'm not sure if this comment should be on the issue or on the PR but could we make the error name easier to read by using the ""full"" name "Regex" instead of "re".

Thanks!

I like the idea of the module name being more precise, however, that would be a very disruptive change and not something that should be done in this PR without any further discussions. Feel free to create another issue/start a discussion over at discuss.python.org to see if you can get any momentum.

@achhina
Copy link
Contributor Author

achhina commented May 29, 2023

Yowza, I didn't expect this to happen.

Out of habit, I rebased instead of merging on upstream/main for new changes since the last time I touched this branch. That seems to have touched all the files and add all the module owners... sorry about that! I'll try to revert this, or might just close this PR and create a new one if I can't salvage it.

@achhina
Copy link
Contributor Author

achhina commented May 29, 2023

Okay, a forced push to the previous commit seemed to fix that. Sorry I know that a force push is not recommended in cpythons workflow, but it seemed to be the easiest thing I could think of instead of trying to revert the previous rebase.

@hugovk I've added the exception renaming to the 3.13 What's New section, do you think I should change this PR from draft to review?

I can also continue the discussions for the new name for the exception in the original issue.

@hugovk
Copy link
Member

hugovk commented May 30, 2023

Yes, I think think is ready for review.

achhina and others added 3 commits June 4, 2023 15:48
@achhina @hugovk
Update Doc/whatsnew/3.13.rst
bc44d0f 
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@achhina @hugovk
Update Misc/NEWS.d/next/Library/2023-02-08-00-43-29.gh-issue-83162.uf…
a2a81b5 
…dI9F.rst

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
@achhina
Merge branch 'main' into pythonGH-83162
d5e2c44
@achhina achhina marked this pull request as ready for review June 4, 2023 19:51
@achhina achhina requested a review from terryjreedy as a code owner June 4, 2023 19:51
@achhina
Copy link
Contributor Author

achhina commented Jun 4, 2023

Yes, I think is ready for review.

Great, done, and fixed the Sphinx warnings. Just need to settle on the new exception name now.

terryjreedy
terryjreedy requested changes Jun 4, 2023
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

How did RESyntaxError suddenly become RECompileError, which I think is worse. Like Barney, I prefer PatternError. The error is in the pattern, not the compilation process.

The pattern in error naming is SomethingError. I dislike multiple capitals, as in JSONDecodeError. Any of JsonError, DecodeError, ever JsonDecodeError would have been better. RegexError would have been OK with me too, but that apparently was not considered.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@achhina
Copy link
Contributor Author

achhina commented Jun 4, 2023

How did RESyntaxError suddenly become RECompileError, which I think is worse.

This was the error message chosen in the original PR that was closed; I had not made any changes to it yet, as I wanted to get a consensus on the name first before going through the effort of renaming.

Like Barney, I prefer PatternError.

Got it, seems to be two votes for PatternError, so unless someone else says otherwise I'll rename it to that when I get some time.

terryjreedy
terryjreedy reviewed Jun 4, 2023
View reviewed changes
Lib/re/_constants.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

Nobody expects the Spanish Inquisition!

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

@bedevere-bot bedevere-bot requested a review from terryjreedy June 7, 2023 09:53
@achhina
Copy link
Contributor Author

achhina commented Aug 22, 2023

@terryjreedy would there be anything else that needs to be done here?

@hugovk
Copy link
Member

hugovk commented Aug 22, 2023

Please could you resolve the merge conflict?

@achhina
Copy link
Contributor Author

achhina commented Aug 22, 2023

Sorry on mobile, will do later today.

@achhina
Copy link
Contributor Author

achhina commented Aug 27, 2023

Hi @hugovk, I've gone ahead and resolved the merge conflicts.

@achhina
Copy link
Contributor Author

achhina commented Dec 3, 2023

Hi, friendly ping @terryjreedy, would you mind taking another look at this PR?

hugovk
hugovk reviewed Dec 3, 2023
View reviewed changes
Doc/library/re.rst Outdated Show resolved Hide resolved
@achhina
Copy link
Contributor Author

achhina commented Dec 3, 2023

Thanks @hugovk, I've gone ahead and made the requested changes.

@achhina achhina requested a review from hugovk December 3, 2023 17:09
hugovk
hugovk approved these changes Dec 3, 2023
View reviewed changes
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you!

@terryjreedy terryjreedy merged commit a01022a into python:main Dec 11, 2023
30 of 31 checks passed
terryjreedy added a commit to terryjreedy/cpython that referenced this pull request Dec 12, 2023
@terryjreedy
[3.12] pythongh-83162: Rename re.error in idlelib (python#101677)
eb8848c 
Backport idlelib part of python#101677 with simple rename.
@terryjreedy terryjreedy mentioned this pull request Dec 12, 2023
Merged
@bedevere-app
Copy link

bedevere-app bot commented Dec 12, 2023

GH-112987 is a backport of this pull request to the 3.12 branch.

terryjreedy added a commit that referenced this pull request Dec 12, 2023
@terryjreedy
[3.12] gh-83162: Rename re.error in idlelib (GH-101677) (#112987)
fd3b894 
Backport idlelib part of #101677 with simple rename.
@miss-islington miss-islington mentioned this pull request Dec 12, 2023
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 12, 2023
@terryjreedy @miss-islington
[3.12] pythongh-83162: Rename re.error in idlelib (pythonGH-101677) (p…
4f64f42 
…ythonGH-112987)

Backport idlelib part of pythonGH-101677 with simple rename.
(cherry picked from commit fd3b894)

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
terryjreedy added a commit that referenced this pull request Dec 12, 2023
@miss-islington @terryjreedy
[3.11] [3.12] gh-83162: Rename re.error in idlelib (GH-101677) (GH-11…
11f9502 
…2987) (#113013)

Backport idlelib part of GH-101677 with simple rename.
(cherry picked from commit fd3b894)

Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@achhina @hugovk
@terryjreedy @aisk
pythonGH-83162: Rename re.error for better clarity. (python#101677)
b4c175c 
Renamed re.error for clarity, and kept re.error for backward compatibility.
Updated idlelib files at TJR's request.
---------

Co-authored-by: Matthias Bussonnier <mbussonnier@ucmerced.edu>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@achhina @hugovk
@terryjreedy @Glyphack
pythonGH-83162: Rename re.error for better clarity. (python#101677)
36e8f13 
Renamed re.error for clarity, and kept re.error for backward compatibility.
Updated idlelib files at TJR's request.
---------

Co-authored-by: Matthias Bussonnier <mbussonnier@ucmerced.edu>
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovk hugovk hugovk approved these changes

@terryjreedy terryjreedy Awaiting requested review from terryjreedy terryjreedy is a code owner

Assignees
No one assigned
Labels
3.13 bugs and security fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@achhina @barneygale @SylvainDe @hugovk @bedevere-bot @terryjreedy