classification
Title: argparse.ArgumentDefaultsHelpFormatter sometimes provides inaccurate documentation of defaults, so they should be overrideable
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: anthonypjshaw, paul.j3, pde
Priority: normal Keywords:

Created on 2016-11-19 00:17 by pde, last changed 2019-05-06 19:07 by anthonypjshaw.

Messages (7)
msg281183 - (view) Author: Peter Eckersley (pde) Date: 2016-11-19 00:17
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 https://github.com/certbot/certbot/issues/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.
msg281184 - (view) Author: Peter Eckersley (pde) Date: 2016-11-19 00:23
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.
msg281189 - (view) Author: Peter Eckersley (pde) Date: 2016-11-19 01:18
Patch is now against the latest Python development tree, and includes test cases:

https://gist.github.com/pde/daec08cadc21bca0d62852020887ee13
msg281590 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2016-11-23 23:03
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').
msg282102 - (view) Author: Peter Eckersley (pde) Date: 2016-11-30 21:09
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)
msg282507 - (view) Author: Peter Eckersley (pde) Date: 2016-12-06 08:44
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.
msg341608 - (view) Author: anthony shaw (anthonypjshaw) * (Python triager) Date: 2019-05-06 19:07
Hi Peter, this proposal would be easier to review as a GitHub Pull Request.
History
Date User Action Args
2019-05-06 19:07:06anthonypjshawsetnosy: + anthonypjshaw
messages: + msg341608
2016-12-06 08:44:44pdesetmessages: + msg282507
2016-11-30 21:09:56pdesetmessages: + msg282102
2016-11-23 23:03:27paul.j3setnosy: + paul.j3
messages: + msg281590
2016-11-19 01:18:15pdesetmessages: + msg281189
2016-11-19 00:23:22pdesettype: enhancement
messages: + msg281184
components: + Library (Lib)
2016-11-19 00:17:09pdecreate