GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-106016: Support customizing of module attributes access with __setattr__/__delattr__ (PEP 726) by skirpichev · Pull Request #108261 · python/cpython

Conversation

@skirpichev

@skirpichev skirpichev commented Aug 22, 2023 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I'm not sure if this does require a PEP (e.g. Brett Cannon on d.p.o discussion thread told me that it "not necessarily"). If so, I've a draft PEP here: https://github.com/skirpichev/peps/tree/setdelattr (And I'm looking for feedback and a sponsor.)

@vstinner

References

  • discussion on d.p.o (the current pr extends the original proposal with the __delattr__ method.)
  • Fast attribute access for module subclasses #103951
  • Issue: PEP 726: Support defining __setattr__() and __delattr__() in a module #106016

📚 Documentation preview 📚: https://cpython-previews--108261.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot mentioned this pull request Aug 22, 2023
PEP 726: Support defining __setattr__() and __delattr__() in a module #106016
Closed
@bedevere-bot bedevere-bot added the awaiting review label Aug 22, 2023
vstinner
vstinner previously requested changes Aug 22, 2023
View reviewed changes
Comment thread Doc/reference/datamodel.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/reference/datamodel.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/reference/datamodel.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/reference/datamodel.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/bad_delattr.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Misc/NEWS.d/next/Core and Builtins/2023-08-22-09-57-59.gh-issue-106016.lpbUq8.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting review label Aug 22, 2023
@bedevere-bot

bedevere-bot commented Aug 22, 2023

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting changes label Aug 22, 2023
@skirpichev

skirpichev commented Aug 22, 2023

Copy link
Copy Markdown
Member Author

@vstinner, I have made the requested changes; please review again. But please see remarks about PyDict_GetItemRef() & PEP 562 test layout above.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Aug 22, 2023
@bedevere-bot

bedevere-bot commented Aug 22, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot
bedevere-bot requested a review from vstinner August 22, 2023 14:00
@vstinner

vstinner commented Aug 22, 2023

Copy link
Copy Markdown
Member

I created PR #108293 to add Lib/test/test_module/ directory. Once it will be merged, you can rebased your PR on top of it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Aug 22, 2023

Copy link
Copy Markdown
Member

Can you please rebase your PR on the main branch to get my #108293 change? (New Lib/test/test_module/ directory.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev and others added 11 commits August 22, 2023 18:11
@skirpichev
pythongh-106016: Support customizing of module attributes access with…
a840465
… __setattr__/__delattr__
@skirpichev @vstinner
Update Doc/reference/datamodel.rst
3e78225
Co-authored-by: Victor Stinner <[email protected]>
@skirpichev @vstinner
Update Doc/reference/datamodel.rst
0a8cec6
Co-authored-by: Victor Stinner <[email protected]>
@skirpichev @vstinner
Update Doc/reference/datamodel.rst
2904f50
Co-authored-by: Victor Stinner <[email protected]>
@skirpichev
Drop PyErr_Clear()
bce1667
@skirpichev
move delattr/setattr declarations
bcafd0d
@skirpichev
m -> mod
0740efb
@skirpichev
Check PyObject_CallFunctionObjArgs return value
74c3d13
@skirpichev
Document changes in the "Other Language Changes" section
83c4c50
@skirpichev
Use PyDict_GetItemRef instead of PyDict_GetItemWithError
7fe6a06
@skirpichev
Move test modules to Lib/test/test_module/
47d93ce
@skirpichev
skirpichev force-pushed the module-setdelattr branch from 7e72a68 to 47d93ce Compare August 22, 2023 15:13
@skirpichev

skirpichev commented Aug 22, 2023

Copy link
Copy Markdown
Member Author

Can you please rebase your PR on the main branch to get my #108293 change?

Yep. Sorry, first I did merge. (BTW, after 7fe6a06 - setattr/delattr declarations should be moved up again I think.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Aug 22, 2023
View reviewed changes
Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev added 2 commits August 23, 2023 05:58
@skirpichev
Avoid ret variable
f3f796d
@skirpichev
+Py_DECREF
f971a22
@vstinner
vstinner dismissed their stale review August 23, 2023 03:18

PR was updated

@skirpichev
Reorder setattr/delattr paths
68eefbd
@skirpichev
skirpichev requested a review from vstinner August 23, 2023 04:08
@skirpichev
Merge branch 'main' into module-setdelattr
048d8bf
vstinner
vstinner reviewed Aug 23, 2023
View reviewed changes
Comment thread Objects/moduleobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_module/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_module/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_module/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Aug 23, 2023

Copy link
Copy Markdown
Member

With proposed fixed, ./python -m test test_module -R 3:3 pass (there is no reference/memory leak).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev @vstinner
Apply suggestions from code review
058412e
Co-authored-by: Victor Stinner <[email protected]>
vstinner
vstinner approved these changes Aug 24, 2023
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Aug 24, 2023
@skirpichev

skirpichev commented Aug 24, 2023

Copy link
Copy Markdown
Member Author

@vstinner, wait! Are you sure this pr doesn't require a PEP?

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev
skirpichev commented Aug 24, 2023
View reviewed changes
Comment thread Lib/test/test_module/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@AA-Turner

AA-Turner commented Aug 24, 2023

Copy link
Copy Markdown
Member

We have a PEP for adding only one attribute (__call__; PEP-713) -- this is a much larger proposal and as such I believe should go through the PEP process, given 713's precedent.

A

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Aug 24, 2023

Copy link
Copy Markdown
Member Author

With proposed fixed, ./python -m test test_module -R 3:3 pass (there is no reference/memory leak).

This seems to be broken on the main branch as well.

We have a PEP for adding only one attribute (call; PEP-713)

And PEP 562 (two attributes). Fair enough.

I believe should go through the PEP process

I've a draft PEP: https://raw.githubusercontent.com/skirpichev/peps/7cc52e7/pep-9999.rst
It lacks a sponsor so far.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
Update Lib/test/test_module/__init__.py
49c82c5
@AA-Turner

AA-Turner commented Aug 24, 2023

Copy link
Copy Markdown
Member

It lacks a sponsor so far.

I'm happy to sponsor if you'd like? Adam Turner <python at quite.org.uk>.

A

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Aug 24, 2023 via email •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author
On Wed, Aug 23, 2023 at 06:21:16PM -0700, Adam Turner wrote: I'm happy to sponsor
Ok. I'll prepare a PR today. UPD: python/peps#3301
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
Merge branch 'main' into module-setdelattr
280fc0e
@skirpichev skirpichev mentioned this pull request Aug 24, 2023
PEP 726: Module __setattr__ and __delattr__ python/peps#3301
Merged
19 of 27 tasks
@skirpichev
Merge branch 'main' into module-setdelattr
dafc546
@vstinner vstinner changed the title gh-106016: Support customizing of module attributes access with __setattr__/__delattr__ gh-106016: Support customizing of module attributes access with __setattr__/__delattr__ (PEP 726) Oct 7, 2023
@vstinner

vstinner commented Oct 7, 2023

Copy link
Copy Markdown
Member

Update: PEP 726 – Module __setattr__ and __delattr__ was written and is being discussed in the PEP section.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@terryjreedy

terryjreedy commented Dec 20, 2023

Copy link
Copy Markdown
Member

Even though Victor closed #106016, I think that this can remain open until the SC decides on the PEP. If it says no, close this. If yes, open a new issue and retarget this by editing the title with the new number. And edit further if required.

AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Dec 21, 2023

Copy link
Copy Markdown
Member Author

If yes, open a new issue and retarget this by editing the title with the new number.

Not sure I got idea of this bureaucracy:) If it's required to have an issue for a pr (as it seems) - we already have an associated issue, #106016.

AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev
Merge branch 'main' into module-setdelattr
9dcacaf
@skirpichev
skirpichev requested a review from ericsnowcurrently as a code owner February 25, 2024 08:28
@skirpichev
Mention PEP in docs, like PEP 562
cd0e4be
@skirpichev

skirpichev commented Feb 25, 2024

Copy link
Copy Markdown
Member Author

Merge conflict was fixed & added the reference to pep in docs.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev

skirpichev commented Feb 28, 2024

Copy link
Copy Markdown
Member Author

Thanks for review and sponsoring the pep.

ericsnowcurrently and vstinner reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@skirpichev skirpichev closed this Feb 28, 2024
@skirpichev
skirpichev deleted the module-setdelattr branch February 28, 2024 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@skirpichev @bedevere-bot @vstinner @AA-Turner @terryjreedy
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.