-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-133653: Fix subclassing HelpFormatter
#133668
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
Conversation
Lib/argparse.py
Outdated
return self.formatter_class( | ||
prog=self.prog, | ||
prefix_chars=self.prefix_chars, | ||
color=self.color, | ||
) | ||
else: | ||
except TypeError: |
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.
TypeError seems a bit generic (you're calling user code).
What do you think about this?
len([v.kind for (k, v) in inspect.signature(self.formatter_class).parameters.items() if k in ('y', 'z') and v.kind in (inspect._ParameterKind.POSITIONAL_OR_KEYWORD, inspect._ParameterKind.KEYWORD_ONLY)]) == 2
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.
That doesn't work: FAILED (failures=2, errors=156)
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.
Wait, it does, obviously need to replace y
and z
:)
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.
I would not like to use inspect.signature()
here. It is not reliable. If it fails (or return incorrect result), you cannot do anything with this. It can only be used in interactive introspection, when errors can be ignored.
Would you prefer the |
No, I think the user code should be changed. They use undocumented feature. |
We can do my original suggestion - make Already two external libraries were broken by this in the first alpha testing. Leaving this as it is will break user code for sure. |
This would not help. The problem is that the user code does not accept any arguments besides |
What's the concern with inspect? That it won't work for some weird function? In that case we can just use the default values for these arguments. |
Another option is to set these fields through a setter method and not via |
See alternative PR #133813. |
The problem was with this code:
We want to set the extra arguments for
HelpFormatter
andargparse
's own subclasses, which don't have their own__init__
s.But many third-party subclasses do have an
__init__
and don't pass onkwargs
. So we can't just check for a subclass ofHelpFormatter
, so let'stry
/except
instead.