-
-
Notifications
You must be signed in to change notification settings - Fork 203
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
help: Alias overriding help-text (like flags), options list human-help before automated infos #361
Conversation
- Renamed the use of builtin `help` in one occasion; - changed 2 TCs to test with 2-tuples; - improved slightly the documentation. See ipython#361 for rational.
+ Mimic behavior of similar libraries (docopt, click) and tools. Part of ipython#361.
Pushed a stylistic help-message modification: put human explanation for opts above automated infos "Before" Help Message:
"After" Help Message:
|
Regarding my last 6a93d8c for putting human option-infos on top: I'm now thinking that it may be appropriate to keep the (regardless, this last commit still has to apply) |
Is there any problem with this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've been very busy, and haven't had time to review these PRs. I made a few comments inline.
@@ -221,17 +221,20 @@ class defaults. | |||
base_classes = ','.join(p.__name__ for p in cls.__bases__) | |||
final_help.append(u'%s(%s) options' % (cls.__name__, base_classes)) | |||
final_help.append(len(final_help[0])*u'-') | |||
for k, v in sorted(cls.class_traits(config=True).items()): | |||
for k, v in sorted(cls.class_own_traits(config=True).items()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this should still be .class_traits
, since it should include all configurables on the class, not just those it didn't inherit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion on that.
As reasoned in this comment, this was also to avoid clutter on --help-all
: since #289 class-parents are printed, so the user may connect the dots and deduce which traits belong to subclasses.
But I will retract it, to keep the --help-all
extra helpfull.
@@ -66,7 +66,7 @@ class MyApp(Application): | |||
|
|||
aliases = Dict({ | |||
('fooi', 'i') : 'Foo.i', | |||
('j', 'fooj') : 'Foo.j', | |||
('j', 'fooj') : ('Foo.j', "`j` terse help msg"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of the help string on traitlets is to specify this help string. Under what circumstances would one set a help string that one didn't want to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is foo --help
and foo --help-all
to print gradually more text for options.
Optimally, with --help
, each option "should" occupy a single line, as most unix-utils do, to waste as little screen estate as possible (ok, we're cannot achieve 1-line bc there are default values, equivalent flags, etc). That is important for apps with a lot of aliases.
The --help-all
is like printing the full man-page.
Of course it is still possible to use the original help-string.
Personally I use a little function that puts in the tuple the 1st line from the full help-msg.
More detailed reasoning on the PR OP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have difficulty with that, since I can't think of any cases where I would want the two messages differ. I see the rationale in the OP, but it's not accurate, for instance:
For flags, it is possible (actually forced) to provide shorter versions
There's a field for a help string only because it's not retrievable from the traits. There is no restriction on its length.
The only difference between --help
output and --help-all
output should be whether the entirety of classes are walked, vs only those items with aliases and flags. The .help
key exists solely for the purpose of populating this output, so I don't understand why it wouldn't be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same reasoning you explained could have been applied also in the flags, like that:
Since .help
keys exist for all options a flag masp to, then just concatenate all of them in one big help-message, and use this for the flag - no need to have a shorter version in a 2-tuple.
But hopefully the case is not like that.
[edit: more accurate numbers concerning my app]
I have a need for this because my app has 15 subcommands 31 configurables and 40 config-params, with 3 levels of nesting. And since i did a trick to merge aliases and flags from parent cmds, at times the output of root foo bar --help
is simply overwhelming, with tens of of aliases & flags.
So I'm striving to use shorter versions to conserve screen estate and help the users see what's important.
I repeat this PR provides an optional functionality, so by default the longer-help message is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, the number of config-params above is an order of magnitude bigger if you include has_traits()
instead of has_own_traits()
. That is why my retracted 4148033 was trying to save space also for --help-all
case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the future reader: describing here cases where it is totally necessary to override the help-msg on the alias: the trait is defined in some base-class, and each alias wants to explain its exact usage.
To keep the API symmetric to flags, add the capability to aliases to override the printed help-msg.
With this PR, the
Application.aliases
dict may contain two-tuples as values like this(alias: (longname, help-text)}
:help
in one occasion;Rational:
Currently code that prints the help-text for the aliases fetches the help-text from the aliased trait, and that is possible since there is a 1-1 correspondence (in contrast to flags where it is 1-n, and thus had to introduce the two-tuple with help-text).
But since this might be long, the end result will be a long help message, scrolling past the end of the screen.
For flags, it is possible (actually forced) to provide shorter versions of the help-messages, and use
--help-all
to read all content, but this flexibility is impossible for aliases.To facilitate comparison, the before and after help messages are provided for the
--config-paths
alias:"Before" Help Messages:
"After" Help Message: