Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-58211: Add tests for __self__ attribute of builtins functions#113575
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-58211: Add tests for __self__ attribute of builtins functions #113575
Uh oh!
There was an error while loading. Please reload this page.
Conversation
adorilson commented Dec 29, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
The check about the f argument type was removed in this commit: python@2c94aa5 Thanks for Pedro Arthur Duarte (pedroarthur.jedi at gmail.com) by the help with this bug.
…#106335) Remove private _PyThreadState and _PyInterpreterState C API functions: move them to the internal C API (pycore_pystate.h and pycore_interp.h). Don't export most of these functions anymore, but still export functions used by tests. Remove _PyThreadState_Prealloc() and _PyThreadState_Init() from the C API, but keep it in the stable API.
AlexWaygood left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
This appears to me to go in the opposite direction to the consensus in #58211 -- the consensus seems to me to be that this is a misfeature that could only be confusing. That doesn't mean that we should necessarily bother changing the current behaviour unless it's actually causing a bug for somebody. But we should only add tests for things which are working in a desirable way currently -- we shouldn't cement undesirable behaviour by adding tests for it.
I'm therefore doubtful that this PR is a good idea. Instead, as I suggested in #113574 (comment), I think we should fix the documentation so that it is more vague about what the value of this attribute might be for builtin functions.
adorilson commented Jan 1, 2024
Yes, I agree. (Although I'm the PR author). The big problem is where desirable behaviour is defined. Furthermore, ignoring other necessary knowledge, I think of the
I don't know if this is a good idea. I guess not. Maybe it is better to remove this attribute from the documentation. Seeing the Terry's comment, the bug is not only the |
serhiy-storchaka commented Jan 1, 2024
There is a code in |
adorilson commented Jan 1, 2024
Do you mean:
? |
serhiy-storchaka commented Jan 2, 2024
I meant considering this PR. |
AlexWaygood left a comment
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.
@adorilson, if you make @serhiy-storchaka's suggested changes, then we can consider merging this:
We can make these tests running only on CPython, and add comments explaining this.
We have a test.support.cpython_only decorator for tests that are only meant to run if the implementation is CPython.
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 |
adorilson commented Mar 16, 2025
I have made the requested changes; please review again I've added the Regarding the comment explaining it, I put a 'See gh-58211' only. Is it enough? |
Thanks for making the requested changes! @AlexWaygood: please review the changes made to this pull request. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
adorilson commented Apr 10, 2025
I have made the requested changes; please review again |
Thanks for making the requested changes! @AlexWaygood: please review the changes made to this pull request. |
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka left a comment
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.
891465f into python:mainUh oh!
There was an error while loading. Please reload this page.
Thanks @adorilson for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…nctions (pythonGH-113575) --------- (cherry picked from commit 891465f) Co-authored-by: Adorilson Bezerra <adorilson@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
GH-132437 is a backport of this pull request to the 3.13 branch. |
…unctions (GH-113575) (#132437) gh-58211: Add tests for the `__self__` attribute of builtins functions (GH-113575) --------- (cherry picked from commit 891465f) Co-authored-by: Adorilson Bezerra <adorilson@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
bedevere-bot commented Apr 12, 2025
|
Although there was an inconclusive discussion in #58211, this PR adds tests to
__self__in built-in callables as it work currently.If was accepted, after PR #113574 is merged we can fix the documentation too (just avoid conflict).