-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
argparse.ArgumentDefaultsHelpFormatter sometimes provides inaccurate documentation of defaults, so they should be overrideable #72928
Comments
|
When argparse lists the default values for cli flags and arguments, it shows argparse's view of the internal Pythonic default, not the default behaviour of the program as a whole. This can be wrong in many cases, as documented at https://bugs.python.org/issue27153 and certbot/certbot#3734. To mitigate this, we should allow the caller to set arbitrary strings that argparse will display to the user as the "default" in the CLI help. I wrote a first patch to do this via a new "help_default" argument that can be added to argparse actions: https://gist.github.com/pde/daec08cadc21bca0d62852020887ee13 The patch was written against argparse.py from Python2.7, but seems to apply and run okay against the Python 3.x version. |
|
One thing I noticed when testing my patch by vendorizing it into the Certbot tree was that if a developer had somehow inherited from a version of argparse.Action that *doesn't* have this patch applied to it, and then passes in instances of those inheriting classes to the new patched version, things will break (because the argparse engine now assumes every action has a help_default attribute, rather than checking for it). That's an unlikely situation but might arise if a developer had managed to use the standard library version of argparse in some places, and the pypi packaged version in other places. It might be possible, but uglier, to write this patch more defensively so that that wouldn't happen; I would appreciate some feedback on whether people think that's necessary or a good idea. |
|
Patch is now against the latest Python development tree, and includes test cases: https://gist.github.com/pde/daec08cadc21bca0d62852020887ee13 |
|
I might accept a patch that tweaks the ArgumentDefaultsHelpFormatter class, but adding a parameter that must propagate through all the Action classes in just plain wrong. Users are confused by the many Action parameters as it is. And based on StackOverFlow questions, I'd say there are lots of custom Action classes. We cannot make a change that might break those! One possibility is to tweak the if '(default` not in action.help:Then the user who is not happy with the '%(default)s` display could just write a help like: help = 'my help text (default: mycustomvalue)'In other words, embed the bypass mechanism entirely in the 'help' string and the Formatter class. The mechanism would have to be something that is simple to explain, and be unlikely to interfere with existing uses of this Formatter. And to play it safe, I'd suggest field testing a SmarterDefaultsHelpFormatter class first - one that implements a change like this, without touching the existing class. Keep in mind that ArgumentDefaultsHelpFormatter is never essential. The argparse developer can always add the '%(default)s` strings to selected 'help' lines. He can even do this in utility functions that know more about the underlying program semantics. I don't think this proposed enhancement gives the end user any added control. If you want to make a stronger case for this enhancement, find other cases where it would be useful. So far you are just arguing that '(default= True)' for a 'store_false' Action may convey the wrong sense to users. As r.david.murray argued, this problem can be minimized with a proper phrasing of the help string. Another thought - 'store_false' is just a subclass of 'store_const'. You could use that class instead with a different set of 'const' and 'default' values. e.g. ('Yes','No', or 'not-False', 'not-True'). |
|
Thanks for your feedback Paul! I agree your proposed implementation strategy would probably be saner; I'll revise the patch to use that approach or something like it. As for the question of necessity, there are definitely more cases than just the store_false ones -- I documented these in the linked Certbot bug but very briefly they include:
|
|
OK, here's another solution following paul.j3's suggestion. I think this is much better: https://gist.github.com/pde/817a00378d3f6ed73747dfffce323ae5 Tests & documentation included. |
|
Hi Peter, this proposal would be easier to review as a GitHub Pull Request. |
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: