-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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-108901: Add bound_arg
to Signature.from_callable()
and signature()
#116559
base: main
Are you sure you want to change the base?
Conversation
…and `signature()`
Co-authored-by: Victor Stinner <[email protected]>
By the way, @pitrou: Do you have an opinion on the parameter name? :-) |
I would prefer to have Lines 1404 to 1413 in 4e5df20
|
skip_bound_arg=False is a double negation, and I'm always confused by double negation :-( bound_arg=True is more straightfoward to me. Previously, the argument was private, no? |
Yes, it was private. It only existed on protected |
skip_bound_arg
to Signature.from_callable()
and signature()
bound_arg
to Signature.from_callable()
and signature()
Done, now it is named |
What's the motivation for this change? I can't find the discussion behind it. My worry is that it only works on some callables, and it's not very clear which ones those are -- or how to make a custom callable (e.g. one implemented in the C API) work like a method. |
I want to deprecate Lines 1403 to 1423 in 2c45148
It will work exactly like this older function. |
Is this behaviour useful, or is it a bug that's kept in |
If *bound_arg* is ``False``, remove ``self`` parameter | ||
from the method signature. |
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.
It's better to describe the non-default behaviour for boolean options (so folks know what they'll get if they request non-default behaviour):
If *bound_arg* is ``False``, remove ``self`` parameter | |
from the method signature. | |
If *bound_arg* is ``True``, report the (already bound) first parameter on bound | |
instance or class methods (usually ``self`` or ``cls``). This emulates the historical | |
behaviour of the deprecated :func:`getfullargspec` function. |
(Describing both behaviours is also an option, but it seemed excessively verbose in this case)
|
||
* Add *bound_arg* parameter to :func:`inspect.Signature.from_callable` | ||
and :func:`inspect.signature`: keep the ``self`` parameter | ||
in the method signature if *bound_arg* is True. |
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.
Reword here to match the updated main function docs (including mentioning the getfullargspec compatibility)
* Add *bound_arg* parameter to :func:`inspect.Signature.from_callable` | ||
and :func:`inspect.signature`: keep the ``self`` parameter | ||
in the method signature if *bound_arg* is True. | ||
:pypi:`inspect313` package has a backport of this feature. |
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.
Maybe make the backport mention a seealso note in the module docs rather than only putting it here?
@@ -0,0 +1,3 @@ | |||
Add *bound_arg* keyword-only parameter to | |||
:func:`inspect.Signature.from_callable` and :func:`inspect.signature`. | |||
If *bound_arg* is ``True``, keep ``self`` parameter in method a signature. |
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.
Mention the getfullargspec compatibility aspect here.
IMO it's useful in general, not only for backward compatibility. Keep self or not should be configurable. |
I think it was primarily a quirk of the way However, the suggested flag isn't hard to maintain, since the complexity is in the branch that corrects the underlying function signature to remove the already bound first parameter (the default behaviour). The "unwrap bound methods" branch is literally just Line 2517 in 3989894
I do think making the flag public is worth including as a step prior to full programmatic deprecation of (The one potential argument I see for a parameter name like |
My worry is that while the flag isn't hard to maintain as an internal detail of how (Unfortunately I'll probably not have time to investigate this worry before beta 1. If the feature is waiting for my review, it won't make it.) |
inspect
module, deprecate old incorrect APIs #108901📚 Documentation preview 📚: https://cpython-previews--116559.org.readthedocs.build/