| 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) *  |
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) * |
Date: 2019-05-06 19:07 |
| Hi Peter, this proposal would be easier to review as a GitHub Pull Request. |
|
|