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.

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-106016: Support customizing of module attributes access with __setattr__/__delattr__ (PEP 726) #108261

Closed
wants to merge 21 commits into from

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Aug 22, 2023

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


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

Doc/reference/datamodel.rst Outdated Show resolved Hide resolved
Doc/reference/datamodel.rst Outdated Show resolved Hide resolved
Doc/reference/datamodel.rst Show resolved Hide resolved
Doc/reference/datamodel.rst Outdated Show resolved Hide resolved
Objects/moduleobject.c Outdated Show resolved Hide resolved
Objects/moduleobject.c Outdated Show resolved Hide resolved
Objects/moduleobject.c Outdated Show resolved Hide resolved
Objects/moduleobject.c Outdated Show resolved Hide resolved
Lib/test/bad_delattr.py 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.

@skirpichev
Copy link
Member Author

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

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@bedevere-bot bedevere-bot requested a review from vstinner August 22, 2023 14:00
@vstinner
Copy link
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.

@vstinner
Copy link
Member

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

@skirpichev
Copy link
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.)

Objects/moduleobject.c Outdated Show resolved Hide resolved
Objects/moduleobject.c Show resolved Hide resolved
Objects/moduleobject.c Outdated Show resolved Hide resolved
Objects/moduleobject.c Outdated Show resolved Hide resolved
Objects/moduleobject.c Outdated Show resolved Hide resolved
@vstinner vstinner dismissed their stale review August 23, 2023 03:18

PR was updated

@skirpichev skirpichev requested a review from vstinner August 23, 2023 04:08
Objects/moduleobject.c Outdated Show resolved Hide resolved
Lib/test/test_module/__init__.py Show resolved Hide resolved
Lib/test/test_module/__init__.py Outdated Show resolved Hide resolved
Lib/test/test_module/__init__.py Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

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

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM!

@skirpichev
Copy link
Member Author

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

@AA-Turner
Copy link
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

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

@AA-Turner
Copy link
Member

It lacks a sponsor so far.

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

A

@skirpichev
Copy link
Member Author

skirpichev commented Aug 24, 2023 via email

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

vstinner commented Oct 7, 2023

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

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

@skirpichev
Copy link
Member Author

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

@skirpichev
Copy link
Member Author

Thanks for review and sponsoring the pep.

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants