◐ Shell
reader mode source ↗
Skip to content

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

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

Copy link
Copy Markdown
Contributor

@erlend-aasland

erlend-aasland commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

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

tiran
tiran requested changes Nov 19, 2020
View reviewed changes

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

Hide comment

See inline comments

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

@bedevere-bot

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

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

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

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

erlend-aasland commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

@bedevere-bot

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
tiran reviewed Nov 19, 2020
View reviewed changes

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

Hide comment

Thanks!

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

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

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

Hide comment

lgtm awesome,

I ran manually some tests and works well.

corona10
corona10 reviewed Nov 19, 2020
View reviewed changes

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

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

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

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.

@bedevere-bot

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

terryjreedy commented Nov 20, 2020

Copy link
Copy Markdown
Member

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

@terryjreedy terryjreedy reopened this Nov 20, 2020
@terryjreedy

terryjreedy commented Nov 20, 2020

Copy link
Copy Markdown
Member

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

@erlend-aasland

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
corona10 approved these changes Nov 20, 2020
View reviewed changes

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

Hide comment

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

@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