Uh oh!
There was an error while loading. Please reload this page.
gh-58211: Add tests for __self__ attribute of builtins functions - #113575
Merged
picnixz merged 13 commits intoApr 12, 2025
Conversation
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.
Member
There was a problem hiding this comment.
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
ContributorAuthor
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
Member
There is a code in |
adorilson
commented
Jan 1, 2024
ContributorAuthor
Do you mean:
? |
serhiy-storchaka
commented
Jan 2, 2024
Member
I meant considering this PR. |
AlexWaygood
requested changes
Jan 3, 2025
AlexWaygood
left a comment
Member
There was a problem hiding this comment.
@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
ContributorAuthor
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. |
skirpichev
approved these changes
Apr 10, 2025
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
ContributorAuthor
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. |
skirpichev
approved these changes
Apr 11, 2025
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
picnixz
approved these changes
Apr 12, 2025
picnixz
enabled auto-merge (squash)
April 12, 2025 11:39
Thanks @adorilson for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
miss-islington pushed a commit
to miss-islington/cpython
that referenced
this pull request
Apr 12, 2025
…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. |
picnixz added a commit
that referenced
this pull request
Apr 12, 2025
…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
|
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).