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

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

Merged
merged 15 commits into from
Nov 20, 2020
Merged

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

merged 15 commits into from
Nov 20, 2020

Conversation

erlend-aasland
Copy link
Contributor

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

@erlend-aasland
Copy link
Contributor Author

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

tiran
tiran requested changes Nov 19, 2020
View reviewed changes
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

See inline comments

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

Modules/_sre.c Outdated Show resolved Hide resolved
Modules/_sre.c Outdated Show resolved Hide resolved
@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.

@erlend-aasland
Copy link
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
Copy link
Member

tiran commented Nov 19, 2020

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
Copy link
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
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

tiran
tiran reviewed Nov 19, 2020
View reviewed changes
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Thanks!

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

corona10
corona10 previously approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm awesome,

I ran manually some tests and works well.

corona10
corona10 reviewed Nov 19, 2020
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

erlend-aasland commented Nov 19, 2020 •
edited
Loading

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
Copy link
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.

@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.

@terryjreedy
Copy link
Member

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

@terryjreedy terryjreedy reopened this Nov 20, 2020
@terryjreedy
Copy link
Member

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

@erlend-aasland
Copy link
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
corona10 approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

@corona10 corona10 merged commit a6109ef into python:master Nov 20, 2020
@erlend-aasland erlend-aasland mentioned this pull request Nov 20, 2020
Closed
@erlend-aasland 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
@adorilson
bpo-1635741: Convert _sre types to heap types and establish module st…
acdf30c 
…ate (PEP 384) (pythonGH-23393)
@serhiy-storchaka serhiy-storchaka mentioned this pull request Apr 15, 2022
Closed
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiran tiran tiran requested changes

@corona10 corona10 corona10 approved these changes

@terryjreedy 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
@erlend-aasland @bedevere-bot @tiran @terryjreedy @corona10 @the-knights-who-say-ni