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
inspect.getfullargspec (etc) incorrectly follows __wrapped__ chains #64883
Comments
In a comment on bpo-17482, Mike Bayer pointed out a backwards incompatibility resulting from changing inspect.getfullargspec (etc) to rely on inspect.signature: they now follow __wrapped__ chains, where previously they ignored them. This means that instead of reporting the exact signature of the *wrapper*, they now report the signature of the wrapped function instead. Since switching these functions from ignoring __wrapped__ to following it is an unintended backwards incompatible change, I'll tweak the code to bypass the unravelling of wrapper chains in the getfullargspec case. |
Attached patch moves the signature to an internal helper that takes an additional flag - whether or not to follow wrapper chains. getfullargspec() now calls this with the flag turned off, and that is passed down to any recursive calls. I'll be offline again until tomorrow evening, so don't feel obliged to wait for me if the patch looks good or just needs minor tweaks before merging. I also noticed a quirk with the getfullargspec -> signature fallback - we still drop the leading arg for partialmethod and other sources of signatures that aren't special cased. That's probably OK though - previously those wouldn't report signatures at all. |
Nick, thanks for writing this patch!
I honestly don't think it is OK. I think we should stick to the behaviour of old 'getfullargspec' consistently, to make its behaviour always predictable. I further tweaked your patch, please review the new one (*_02.diff) The changes are relatively simple -- I've added a new bool flag to '_signature_internal' -- skip_bound_arg. When it is False, the logic for skipping bound args is off. It also made 'getfullargspec' implementation simpler. Now 'getfullargspec()' should behave always like it did before + it will handle more callables. Larry, if you have some time for this, I'd be glad to receive your feedback on this. This issue is quite important. |
Your version looks good to me, Yury (and good idea to add the second flag). |
Thanks, Nick. Do you want me to commit the patch? |
Yeah, I think push this (with NEWS etc) for 3.4.1, then create the cherry |
My only question: do they need to be independent flags? ISTM that they're always either both True or both False. |
I'd keep them independent solely to make the code easier to read/audit. As, for instance, a combined argument won't make sense for someone who just inspects the code of '_signature_from_builtin()' helper. |
I think in 3.5 it may actually make sense to expose these as options The only plausible combined name would also be something like "legacy", and |
Committed in 65fb95578f94 |
Issue bpo-20688 to track cherry-picking this into 3.4.0 |
+1 on thinking about exposing your 'follow_wrapper_chains' option in 3.5. I'll create an issue. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: