-
Notifications
You must be signed in to change notification settings - Fork 32
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
Show desired super command flags in command help. #60
Conversation
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.
This looks great! I'm not sure you need the lookup in the VisitAll call though.
cmd.go
Outdated
// HelpWithSuperFlags renders i's content, along with documentation for any | ||
// flags defined in both command and its super command flag sets. | ||
// Only super command flags defined in i.ShowSuperFlags are displayed, if found. | ||
// It calls FLagSet.SetOutput(ioutil.Discard). |
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 think you mean that it leaves the flagset in a state where it won't output anything, is that right? It would be clearer to say that in a higher-level way - this just repeats the code.
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'll just remove it from the call I've added. cut and paste strikes again :D
|
||
// ShowSuperFlags contains the names of the 'super' command flags | ||
// that are desired to be shown in the sub-command help output. | ||
ShowSuperFlags []string |
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 guess we never have any cases where we have one command that can work under multiple supercommands, do we?
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, we do not :)
cmd.go
Outdated
emptyFlag := &gnuflag.Flag{} | ||
superF.VisitAll(func(flag *gnuflag.Flag) { | ||
if contains(flag.Name) { | ||
if found := filteredSuperF.Lookup(flag.Name); found == nil || found == emptyFlag { |
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.
Can a flagset have multiple flags with the same name? From reading VisitAll it doesn't seem like it - it iterates through FlagSet.formal which is a map of the names.
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.
Yes, I have copied this code from my previous iteration on the subject. When the flagset is re-used, the addition of the flag name that already exists would throw an error.
I do not think it is a big deal to have this safe-guard but I will remove it.
|
When reading the help for a sub-command, it is also desirable to see supported options for super command.
For example, for 'juju help add-model', we'd want to see https://pastebin.ubuntu.com/p/yh59W4X68h/
This change enables to specify what super options are to display via info.ShowSuperFlags and adds logic to actual help display. We are grouping super command's options as 'global' and sub-command's options as 'command'.
A new method was added info.HelpWithSuperFlags. For backward compatibility, old info.Help still exists but delegates to the new call.