-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
gh-122358: Remove re._compile #122357
base: main
Are you sure you want to change the base?
gh-122358: Remove re._compile #122357
Conversation
wimglenn
commented
Jul 27, 2024
•
edited
Loading
edited
- Issue: Refactor - remove re._compile? #122358
4e06a9c
to
e6506a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, GitHub is making the diff look weird.
Please add a news entry. If you use the blurb CLI, please upgrade to 1.2: https://discuss.python.org/t/new-blurb-1-2-please-upgrade/59159 |
Move implementation directly into re.compile. Simplifies tracebacks and reduces call stack.
@hugovk Blurbed it, though there is no user-facing change here. |
Thanks! Also please don't force push in this repo, we squash merge everything at the end: https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide |
Misc/NEWS.d/next/Library/2024-08-04-13-22-32.gh-issue-122358.wpnaq4.rst
Outdated
Show resolved
Hide resolved
Lib/test/test_re.py
Outdated
@@ -1227,7 +1227,7 @@ def test_pickling(self): | |||
pickled = pickle.dumps(oldpat, proto) | |||
newpat = pickle.loads(pickled) | |||
self.assertEqual(newpat, oldpat) | |||
# current pickle expects the _compile() reconstructor in re module | |||
# previous pickles may expect the _compile() reconstructor in re module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding a test for pickles that expect _compile()
? ("No" is a valid answer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it in df79dd5
…pnaq4.rst Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
There are arguments against this change. See details in my comment in the issue. |
This uses the warnings feature skip_file_prefixes added in 3.12 python#100840
@serhiy-storchaka ping? anything more to do here? |