-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
update help output suggestions to display correct cmd format #10172
update help output suggestions to display correct cmd format #10172
Conversation
@juanvallejo no longer WIP? |
@fabianofranz Ah, the current pushed changes are complete, but I was a bit hesitant to remove the "WIP" tag in case anyone else had a better solution |
@juanvallejo ok, let me take a look. |
@fabianofranz Hm, do you think it would be a good idea to make a separate function that "parses" command groups for these two commands ( EDIT: By "parse" I meant making sure command groups get printed similar to how they are printed at the moment using the root command template |
What about changing how we suggest it, to the
I think the flag is a more standard way of accessing help anyway. |
Apart this bug, I think this templating system could make good use of some refactorings. It's a little confusing and hard to follow. As far as possible we shouldn't need to mark stuff, for example the help template should just print separate groups if there are groups but print "Available Commands" not grouped if there aren't, etc. |
I agree, I think it might be a good idea to not have child commands "act as root commands" if they are only going to make use of the same usage template as root commands.
Hm, this could also work, I was finishing working on this current solution at the time, but would also be on board with doing this. Could you take a look at the new commit, and see if we could use these changes as well? Below is the new output:
These changes support this format recursively, so if a nested command later on is made to "act as root command", it would keep this correct format as well |
[test] |
@openshift-bot, the last build failed from the following flakes:
re[test] |
I'm not opposed to the new version, but I think it's unnecessary complexity. Unless I'm missing something, just changing it to Feel free to disagree, but I'd go with the simpler solution and eventually work on a refactoring to make this better structured. |
ed28cee
to
16e318e
Compare
Evaluated for origin test up to 16e318e |
@fabianofranz Thanks for the feedback!
I think this is a very valid point, I have gone ahead and updated the I went ahead and saved the other code locally in case there is an actual use-case for it in the future :) |
Sounds good, thanks! LGTM, [merge]. |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7611/) (Image: devenv-rhel7_4776) |
Evaluated for origin merge up to 16e318e |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7589/) |
Related BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1313198
When displaying help output for commands that invoke the templates.ActsAsRootCommand method, suggested commands are displayed out of order:
$ oc set -h
Use "oc set help " should be Use "oc help set "
When invoking help for sub-commands, such as
oc set triggers -h
, the suggested line for Use "oc options"... adds on the immediate parent command:Use "oc set options"...
A suggested solution for this would be to display help output for these commands without calling
templates.ActsAsRootCommand
method, and use a non-root usage template instead:Sub-commands also display the correct usage suggested by the two bottom lines. The only immediate difference is that all available sub-commands are not formatted in groups.
cc @deads2k @fabianofranz @liggitt
EDIT: The only commands currently affected by this are
oc set -h
andoadm policy -h
. If we still wish to print available sub-commands for these commands in groups, we could make a function elsewhere that does this