← 返回首页
bpo-1635741: Convert _sre types to heap types and establish module state (PEP 384) by erlend-aasland · Pull Request #23393 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

bpo-1635741: Convert _sre types to heap types and establish module state (PEP 384)#23393

Merged
corona10 merged 15 commits into
python:masterfrom
erlend-aasland:bpo-1635741/sre
Nov 20, 2020
Merged

bpo-1635741: Convert _sre types to heap types and establish module state (PEP 384)#23393
corona10 merged 15 commits into
python:masterfrom
erlend-aasland:bpo-1635741/sre

Conversation

erlend-aasland commented Nov 19, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

erlend-aasland commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

@vstinner / @corona10: Would you mind reviewing this?

tiran requested changes Nov 19, 2020
View reviewed changes

tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

See inline comments

While you are working on the module, please remove PyModule_GetDict() and replace the code with PyModule_AddStringConstant().

Comment thread Modules/_sre.c Outdated Show resolved Hide resolved
Comment thread Modules/_sre.c Outdated Show resolved Hide resolved

bedevere-bot commented Nov 19, 2020

Copy link
Copy Markdown

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.

erlend-aasland commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

See inline comments

While you are working on the module, please remove PyModule_GetDict() and replace the code with PyModule_AddStringConstant().

I could merge #23101 into this PR. Would that be acceptable?

tiran commented Nov 19, 2020

Copy link
Copy Markdown
Member

See inline comments
While you are working on the module, please remove PyModule_GetDict() and replace the code with PyModule_AddStringConstant().

I could merge #23101 into this PR. Would that be acceptable?

+1, good idea

You'll get bonus points for using PyModule_AddObjectRef.

erlend-aasland commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

You'll get bonus points for using PyModule_AddObjectRef.

GH-23101 merged with commit 38e3cd7.

Adjust idlelib tiptest …
a6bc481
from Lib/idlelib/idle_test/test_calltip.py: The tests of builtins may break if inspect or the docstrings change, but a red buildbot is better than a user crash (as has happened). For a simple mismatch, change the expected output to the actual.

erlend-aasland commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

bedevere-bot commented Nov 19, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

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

20 hidden items Load more…
tiran reviewed Nov 19, 2020
View reviewed changes

tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Thanks!

I'm leaving the final ack to @corona10 and @vstinner

corona10 previously approved these changes Nov 19, 2020
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

lgtm awesome,

I ran manually some tests and works well.

corona10 reviewed Nov 19, 2020
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

But I want to know why this behavior is changed.

master

>>> import re >>> p = re.compile('') >>> p.sub.__doc__ 'Return the string obtained by replacing the leftmost non-overlapping occurrences of pattern in string by the replacement repl.'

PR

>>> import re >>> p = re.compile('') >>> p.sub.__doc__ None

erlend-aasland commented Nov 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

But I want to know why this behavior is changed.

I find that odd as well. At first, I though it was just the AC that messed up the argument list, but it's not. The docstring is clearly there in line 1214. Try this:

>>> import re >>> p = re.compile('') >>> help(p.sub)

It seems to happen only with methods with the defining_class converter. Could it be a bug in AC?

UPDATE

Seems like dbm has the same problem. For example, the _dbm.dbm.keys method, which also has a defining_class converter, and is a method defined on a type belonging to the module:

>>> import dbm >>> d = dbm.open('test.db', 'c') >>> d <_gdbm.gdbm object at 0x103eeb500> >>> d.keys.__doc__ # <= this returns None iso. the docstring as defined in AC >>> help(d.keys) # <= this displays the docstring as defined in AC

erlend-aasland commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

@corona10 or @tiran can you remove Terry J. Reedy as reviewer? Adjusting the idlelib test was a bad idea. Dong-hee Na is right; there's something fishy about the __doc__ attributes for some of the methods.

terryjreedy requested changes Nov 19, 2020
View reviewed changes
Comment thread Lib/idlelib/idle_test/test_calltip.py Outdated Show resolved Hide resolved

bedevere-bot commented Nov 19, 2020

Copy link
Copy Markdown

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.

terryjreedy commented Nov 20, 2020

Copy link
Copy Markdown
Member

Actually, tests should pass now because test runners will use revised idlelib.

terryjreedy reopened this Nov 20, 2020

terryjreedy commented Nov 20, 2020

Copy link
Copy Markdown
Member

@tiran The IDLE test failure is gone. Do you still want changes?

erlend-aasland commented Nov 20, 2020

Copy link
Copy Markdown
Contributor Author

@corona10 IDLE has been fixed, so tests pass fine now. I'll file an issue about the missing __doc__ strings in builtin methods.

corona10 approved these changes Nov 20, 2020
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

lgtm

corona10 merged commit a6109ef into python:master Nov 20, 2020
erlend-aasland mentioned this pull request Nov 20, 2020
Closed
erlend-aasland deleted the bpo-1635741/sre branch November 20, 2020 15:02
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
bpo-1635741: Convert _sre types to heap types and establish module st… …
acdf30c
…ate (PEP 384) (pythonGH-23393)
serhiy-storchaka mentioned this pull request Apr 15, 2022
Closed
kylotan mannequin mentioned this pull request Sep 19, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tiran tiran requested changes
corona10 corona10 approved these changes
terryjreedy terryjreedy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Footer

© 2026 GitHub, Inc.