move formatting of defaults to FormatterClass#65
move formatting of defaults to FormatterClass#65MikkelSchubert wants to merge 6 commits intotyped-argparse:mastertyped-argparse/typed-argparse:masterfrom MikkelSchubert:default_formatter_classMikkelSchubert/typed-argparse:default_formatter_classCopy head branch name to clipboard
Conversation
- allows SUPPRESS to hide help per argument - allows overriding the defaults formatting per parser limit: auto_default_help is only respected by the default FormatterClass
Do you have an example use case, when suppressing the help text of an argument may be desirable? From a UX perspective this feature looks a bit dubious to me. Why should the help keep information secret from the user / how can the user make use of an argument if it is hidden? Is the main use case when it is so obvious that mentioning it is detrimental? |
|
I've used
|
|
The CI is currently failing since the output of The reason is that help text is now generated later, from an It doesn't seem possible to recreate the old behavior exactly, but I am also not sure it is desirable to recreate as since it is somewhat inconsistent: The args The two options I can see are a) accept the change and always print a default for boolean options (unless disabled via Personally I'm more in favor of not showing the default for booleans. The effect of setting a boolean option will normally be explained by the |
|
First of all, sorry for the delay on this one! I was offline for a few days, and didn't had time to look into it in detail yet.
Yes I'd also say that omitting default values in general for switches is more sensible. Especially with negated flags, like say |
| not isinstance(action.help, DefaultHelpFormatter.Unformatted) | ||
| and action.help is not None | ||
| and action.default is not None | ||
| and action.default is not argparse.SUPPRESS |
There was a problem hiding this comment.
There are a few things that I don't understand here yet:
-
It looks like we are going into the else branch if
helpisDefaultHelpFormatter.Unformattedor if it isNone, right? Doesn't that mean thatDefaultHelpFormatter.Unformattedis unnecessary? I.e., could the user achieve the same by settinghelptoNoneanyway? -
What is the purpose of comparing
action.defaulttoargparse.SUPPRESS? In general it seems thatargparse.SUPPRESShas multiple roles in argparse:
- It can be used as a value for the
helpargument to suppress the help text. - It also can be used for the
argument_defaultanddefaultarguments (for the parser or add_argument). This usage has a special meaning that the field will be omitted from theargparse.Namespaceinstance.
The latter usage is problematic from the typed_argparse perspective, because we rely on arguments always being generated in the Namespace instance. Using argparse.SUPPRESS in the default position actually breaks argument parsing currently:
import argparse
from typing import Optional
import typed_argparse as tap
class Args(tap.TypedArgs):
foo: Optional[str] = tap.arg(default=argparse.SUPPRESS)
def run(args: Args):
print(args)
if __name__ == "__main__":
tap.Parser(Args).bind(run).run()Fortunately it doesn't type check for other types than str, because its a string based sentinel value. I'll probably have to catch that, i.e., the default value '==SUPPRESS==' for strings is kinda forbidden 😕
This makes we wonder why to compare with action.default here though. I would have understood if there is a comparison like action.help == argparse.SUPPRESS but surprisingly that doesn't seem necessary - so the filtering actually happens internally in argparse? If so, shouldn't we remove action.default is not argparse.SUPPRESS from the condition, if we cannot support it in the default position anyway?
There was a problem hiding this comment.
Don't worry finding the time for this. There's no hurry.
-
DefaultHelpFormatter.Unformattedis a bit of a hack to support theauto_default_helpoption and is only used ifhelpis not None (see https://github.com/MikkelSchubert/typed-argparse/blob/8a76abd4166a1bcbdd16554ce56851516bcb42be/typed_argparse/parser.py#L480). The problem is thatauto_default_helpisn't available to theHelpFormatterinstance, so instead I convert the (not None) help string to an instance of a string subclass. This allows me to check whether or not the user setauto_default_help=Truewithout breaking stuff in argparse, wherehelpis expected to be a string. There may be a better way of doing this, but I couldn't figure one out. -
I agree with all your points regarding
argparse.SUPPRESS, but it needs to be handled here simply because argparse makes use of it even if we don't. I should probably add a comment about that. If you don't check for it in theHelpFormatterthen you get output like this from (from an unmodifiedexamples/high_level_api/basic_usage.py):-h, --help show this help message and exit [default: ==SUPPRESS==]
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #65 +/- ##
==========================================
+ Coverage 92.89% 93.00% +0.10%
==========================================
Files 9 10 +1
Lines 704 715 +11
==========================================
+ Hits 654 665 +11
Misses 50 50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This patch moves the formatting of default values into a new
FormatterClassthat is then set as the default forParser.This was originally motivated by the observation that
help=SUPPRESSdid not work to hide arguments from -h/--help output (due to being mangled by_generate_help_text), and the subsequent realization that you cannot easily override the formatting of default values. Currently you'd have to setauto_default_helpfor every argument in addition to supplying aFormatterClass.I tried to keep the
auto_default_helpfunctionality intact, but I didn't see obvious way to generalize it. So it'll only affect the formatter class I implemented. I'm not really happy with that part, but personally I'd probably just removeauto_default_helpentirely.