Skip to content
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

Open
pde mannequin opened this issue Nov 19, 2016 · 7 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@pde
Copy link
Mannequin

pde mannequin commented Nov 19, 2016

BPO 28742
Nosy @wimglenn, @pde, @tonybaloney

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:

assignee = None
closed_at = None
created_at = <Date 2016-11-19.00:17:09.209>
labels = ['3.7', 'type-feature', 'library']
title = 'argparse.ArgumentDefaultsHelpFormatter sometimes provides inaccurate documentation of defaults, so they should be overrideable'
updated_at = <Date 2021-06-03.18:03:15.796>
user = 'https://github.com/pde'

bugs.python.org fields:

activity = <Date 2021-06-03.18:03:15.796>
actor = 'wim.glenn'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2016-11-19.00:17:09.209>
creator = 'pde'
dependencies = []
files = []
hgrepos = []
issue_num = 28742
keywords = []
message_count = 7.0
messages = ['281183', '281184', '281189', '281590', '282102', '282507', '341608']
nosy_count = 4.0
nosy_names = ['paul.j3', 'wim.glenn', 'pde', 'anthonypjshaw']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue28742'
versions = ['Python 3.7']

@pde
Copy link
Mannequin Author

pde mannequin commented Nov 19, 2016

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.

@pde pde mannequin added the 3.7 (EOL) end of life label Nov 19, 2016
@pde
Copy link
Mannequin Author

pde mannequin commented Nov 19, 2016

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.

@pde pde mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Nov 19, 2016
@pde
Copy link
Mannequin Author

pde mannequin commented Nov 19, 2016

Patch is now against the latest Python development tree, and includes test cases:

https://gist.github.com/pde/daec08cadc21bca0d62852020887ee13

@paulj3
Copy link
Mannequin

paulj3 mannequin commented Nov 23, 2016

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 %(default)s test to something like:

     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').

@pde
Copy link
Mannequin Author

pde mannequin commented Nov 30, 2016

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:

  • Programs where the default value of a variable is "Ask interactively" if no flag is provided
  • Cases where the default value is the result of some complicated computation (for instance, reading it from a defaults file)

@pde
Copy link
Mannequin Author

pde mannequin commented Dec 6, 2016

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.

@tonybaloney
Copy link
Mannequin

tonybaloney mannequin commented May 6, 2019

Hi Peter, this proposal would be easier to review as a GitHub Pull Request.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@erlend-aasland erlend-aasland removed the 3.7 (EOL) end of life label May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
Status: Features
Development

No branches or pull requests

1 participant