bpo-14965: Proxy super().x = y and del super().x by habnabit · Pull Request #26194 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Arguably this is a bugfix, and therefore should be backported into the 3.10 branch as well.
This patch was originally contributed by Daniel Urban, whose summary
follows:
I'm attaching a patch implementing
super.__setattr__(and__delattr__).The implementation in the patch only works, if super can find a data
descriptor in the MRO, otherwise it throws anAttributeError. As it
can be seen in the tests, in some cases this may result in
counter-intuitive behaviour. But I wasn't able to find another
behaviour, that is consistent with bothsuper.__getattr__and normal__setattr__semantics.
https://bugs.python.org/issue14965
This patch was originally contributed by Daniel Urban, whose summary follows:
I'm attaching a patch implementing super.setattr (and delattr).
The implementation in the patch only works, if super can find a data descriptor in the MRO, otherwise it throws an AttributeError. As it can be seen in the tests, in some cases this may result in counter-intuitive behaviour. But I wasn't able to find another behaviour, that is consistent with both super.getattr and normal setattr semantics.
Attaching a NEWS fragment in a moment.
File "/home/runner/work/cpython/cpython/Lib/test/test_doctest.py", line 671, in test.test_doctest.test_DocTestFinder.non_Python_modules
Failed example:
816 < len(tests) < 836 # approximate number of objects with docstrings
This newly-failing test does appear to be related to the new method super_setattro. I'm not familiar with the new changes to the C API. Does a docstring need to be added to the new super.__setattr__?
#python-dev suggests that this is a coincidental failure and I should increase the numbers from 816 and 836. Should I do that in this change or wait for another PR to bump it?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch looks like Daniel Urban's super_setattr.patch. You cannot take somebody else's code and submit it under your name. Code should be attribute to the original author. We also make sure that Daniel has signed the CLA.
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.
Can you please explain how you’d like me to attribute it? The original author’s name is listed in the NEWS fragment and the commit message already.
On Mon, May 17, 2021 at 23:03 Christian Heimes ***@***.***> wrote: ***@***.**** requested changes on this pull request. The patch looks like Daniel Urban's super_setattr.patch. You cannot take somebody else's code and submit it under your name. Code should be attribute to the original author. We also make sure that Daniel has signed the CLA. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#26194 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFFMCRTHPUED5TKOKKDDTTOGHBXANCNFSM45A7Y27A> .
In fact, I neither want nor need my name on this in the first place. This patch has been sitting stale since 2012 without movement.
On Mon, May 17, 2021 at 23:08 hab ***@***.***> wrote: Can you please explain how you’d like me to attribute it? The original author’s name is listed in the NEWS fragment and the commit message already. On Mon, May 17, 2021 at 23:03 Christian Heimes ***@***.***> wrote: > ***@***.**** requested changes on this pull request. > > The patch looks like Daniel Urban's super_setattr.patch. You cannot take > somebody else's code and submit it under your name. Code should be > attribute to the original author. We also make sure that Daniel has signed > the CLA. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#26194 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAFFMCRTHPUED5TKOKKDDTTOGHBXANCNFSM45A7Y27A> > . >
@tiran Daniel Urban claims on bpo-14965 that they've already signed a contributor agreement. What's the next step?
This PR is stale because it has been open for 30 days with no activity.
What's the next step here? How can we get this merged?
All commit authors signed the Contributor License Agreement.
Really? I'm sure I signed the CLA years ago; I've had patches merged before.
Well, signed again now. Looks like there's another merge conflict. Will anything happen if I actually update the patch?
Well, signed again now. Looks like there's another merge conflict. Will anything happen if I actually update the patch?
I've already updated the patch here: #29950
If you update this one, I assume recent discussion will move here from the new one.
(ignore my previous response, I thought you were commenting on my PR instead of yours)