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

bpo-14965: Proxy super().x = y and del super().x #26194

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

habnabit
Copy link
Contributor

@habnabit habnabit commented May 17, 2021

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

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.
@habnabit
Copy link
Contributor Author

Attaching a NEWS fragment in a moment.

@habnabit
Copy link
Contributor Author

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__?

@habnabit
Copy link
Contributor Author

#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?

Copy link
Member

@tiran tiran left a comment

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.

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

@habnabit
Copy link
Contributor Author

habnabit commented May 17, 2021 via email

@habnabit
Copy link
Contributor Author

habnabit commented May 17, 2021 via email

@habnabit
Copy link
Contributor Author

@tiran Daniel Urban claims on bpo-14965 that they've already signed a contributor agreement. What's the next step?

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 28, 2021
@SwooshyCueb
Copy link

What's the next step here? How can we get this merged?

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 8, 2022
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.

6 participants