GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||
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
|
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@vstinner, I have made the requested changes; please review again. But please see remarks about PyDict_GetItemRef() & PEP 562 test layout above. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Can you please rebase your PR on the main branch to get my #108293 change? (New Lib/test/test_module/ directory.) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
… __setattr__/__delattr__
Co-authored-by: Victor Stinner <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
7e72a68 to
47d93ce
Compare
Yep. Sorry, first I did merge. (BTW, after 7fe6a06 - setattr/delattr declarations should be moved up again I think.) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
With proposed fixed, |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Victor Stinner <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@vstinner, wait! Are you sure this pr doesn't require a PEP? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
We have a PEP for adding only one attribute ( A |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
This seems to be broken on the main branch as well.
And PEP 562 (two attributes). Fair enough.
I've a draft PEP: https://raw.githubusercontent.com/skirpichev/peps/7cc52e7/pep-9999.rst |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I'm happy to sponsor if you'd like? A |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Update: PEP 726 – Module |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Merge conflict was fixed & added the reference to pep in docs. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for review and sponsoring the pep. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.