help: simplify implementation of help subsystem #1139
Conversation
It was fixed with PR #1132 |
elhimov
force-pushed
the
elhimov/gh-1136-display-external-commands-only-in-top-level-help
branch
from
6fc697a to
a8ce4a8
Compare
elhimov
changed the title
elhimov
force-pushed
the
elhimov/gh-1136-display-external-commands-only-in-top-level-help
branch
2 times, most recently
from
85da23c to
5a70d95
Compare
Initially I assumed to solve this task in 2 steps: first refactor help-related code and then fix the mentioned problem. Since the problem part is fixed I adjusted labels to turn bug-task into refactoring-task. |
elhimov
changed the title
elhimov
force-pushed
the
elhimov/gh-1136-display-external-commands-only-in-top-level-help
branch
from
5a70d95 to
623c32e
Compare
Комплишн у хелпа теперь работает и для сложных команд. То есть раньше варианты предлагались только для топ-команд, теперь если набрать Наверное лучше было бы отдельным реквестом это изменение делать. И по-хорошему подкрепить тестами, чтобы потом регрессий не допускать. |
Давайте тогда отобразим это в CHANGELOG.md. |
cli/cmd/external.go
Outdated
| DisableFlagParsing: true, | ||
| } | ||
| cmd.SetHelpFunc(func(cmd *cobra.Command, args []string) { | ||
| cmd.Print(manifest.Help) |
There was a problem hiding this comment.
It won't work that way because, now and in the near future: manifest.Help == "".
Need a check, if the manifest.Help string is empty, then call an external command with the --help flag.
But this should only be done when the user has requested help for such external command, without this, all modules should not be asked about this information on every start of tt.
There was a problem hiding this comment.
Sorry, but it seems that I'm missing something.
I see following flow now:
modules.GetModulesInfo
makeManifest() <- for each discovered module
readManifest() or fillManifest()
readManifest() looks like modern approach and it gets "Help" directly from the manifest file. And according to:
if mf.Help == "" {
return mf, fmt.Errorf("help field is mandatory for module Manifest")
}
"Help" field is mandatory, because any returned error leads to skipping the module.
fillManifest() looks like an approach that provides a kind of backward compatibility. It fills "Help" field by calling external module with --help and it is executed unconditionally for every external module at tt start.
Finally it means that if external module entry exists its Help field is filled properly, so I just use it. Is it wrong or are we going to change this behavior in the nearest future?
There was a problem hiding this comment.
Sorry I've just found that under fillManifest() the corresponding external module is executed with the --description flag, not --help. Anyway it seems we get .Help field from its output, so it confuses me even more )
There was a problem hiding this comment.
Yea, it does cause a bit of an embarrassment, but that kind of history has been drawn out since the previous revision of the modules.
There was a problem hiding this comment.
elhimov
force-pushed
the
elhimov/gh-1136-display-external-commands-only-in-top-level-help
branch
from
623c32e to
9d68797
Compare
Убрал эти изменения отсюда, сделаю отдельным реквестом. |
Following changes are applied: - the main help command is implemented in a way similar to the other commands (choice between internal and external variation of the command is handled with the common handler cmd.RunModuleFunc) - help for every external command is tied to a command itself over cmd.SetHelp function rather than add separate help subcommand to the main help command (it allows to obtain help for internal and external commands uniformly) - corresponding Cobra commands are created for all available external commands rather than only for the invoked one (being combined with the uniform handling mentioned above it allows to make implementation of the internal variation of help command trivial) - external commands are also implemented over the common handler cmd.RunModuleFunc to reduce code duplication Close #1136
elhimov
force-pushed
the
elhimov/gh-1136-display-external-commands-only-in-top-level-help
branch
from
9d68797 to
c866f03
Compare
Following changes are applied:
commands (choice between internal and external variation of the
command is handled with the common handler cmd.RunModuleFunc)
cmd.SetHelp function rather than add separate help subcommand to
the main help command (it allows to obtain help for internal and
external commands uniformly)
commands rather than only for the invoked one (being combined with
the uniform handling mentioned above it allows to make implementation
of the internal variation of help command trivial)
cmd.RunModuleFunc to reduce code duplication
I didn't forget about:
Related issues:
Closes #1136