Skip to content
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

Add groups for commands in help #1003

Merged
merged 3 commits into from Oct 10, 2022
Merged

Add groups for commands in help #1003

merged 3 commits into from Oct 10, 2022

Conversation

aawsome
Copy link
Contributor

@aawsome aawsome commented Dec 14, 2019

Adds the possibility to group the commands in the help message.

closes #836
closes #1271

@jharshman jharshman added the kind/feature A feature request for cobra; new or enhanced behavior label Dec 23, 2019
@jharshman
Copy link
Collaborator

@aawsome I think this is a good addition. It's something I can put on the roadmap after 1.0.0 is released.

@jharshman jharshman modified the milestone: 2.0.0 Dec 23, 2019
@CLAassistant
Copy link

CLAassistant commented Mar 6, 2020

CLA assistant check
All committers have signed the CLA.

@mhewedy
Copy link

mhewedy commented Apr 25, 2020

Any updates on this? or any workarounds exist?

@github-actions
Copy link

github-actions bot commented Jul 8, 2020

This PR is being marked as stale due to a long period of inactivity

@aawsome
Copy link
Contributor Author

aawsome commented Jul 8, 2020

I rebased this PR.
Now that 1.0.0 is out, is there anything I can do to get this PR merged?

@itskingori
Copy link

itskingori commented Jul 8, 2020

@aawsome Maybe we can nudge @jharshman for some visibility. I think should be able to provide some direction.

@umarcor
Copy link
Contributor

umarcor commented Jul 8, 2020

@aawsome would you mind adding some tests for this feature?

@aawsome
Copy link
Contributor Author

aawsome commented Jul 8, 2020

Added some tests. Almost all new code is covered by these tests.

The exception is AddCommandGroup when helpCommand is manually set.
(In this case most people would anyway directly specify the Group within the Command definition)

@aawsome
Copy link
Contributor Author

aawsome commented Jul 19, 2020

Is there anything I can do to help getting this PR merged?

@nacx
Copy link

nacx commented Feb 7, 2021

This has been open for a while and would be pretty nice to have this merged. Any chance anyone could have a look at it?

@itskingori
Copy link

@aawsome Branch has conflicts. Could you rebase? So that if it's ready by the time @jharshman has a another look?

@aawsome
Copy link
Contributor Author

aawsome commented Feb 8, 2021

@aawsome Branch has conflicts. Could you rebase? So that if it's ready by the time @jharshman has a another look?

is rebased.

@yuvalsosman
Copy link

Hey, Any updates regarding this feature?

@umarcor
Copy link
Contributor

umarcor commented Oct 11, 2021

@yuvalsosman, see #1496.

@MichaelMure
Copy link
Contributor

Also close #1271

@jpmcb jpmcb added this to the 2.0.0 milestone Dec 8, 2021
@jpmcb jpmcb removed the kind/stale label Dec 8, 2021
@marckhouzam
Copy link
Collaborator

2. If all commands are assigned to groups, we still see "Available Commands:" at the top but there are no commands under it.  Do we want that, or should we not show it in this case?  In truth, it is not that bad as it looks like it is a heading for all the groups.  However, I wonder if users will like that? (I've seen users refuse to upgrade to a Cobra version because the new `completion` command had its help start with a lower case instead of an uppercase smile )

I thought to leave this as a header. It can be of course removed, but that would require a bit of computation to check if all commands have the Group set. Should I do this?

Leave it for now and we'll see how we feel on the next review.

3. If a command is not part of a group it ends up under "Available Commands:", with the Groups listed after.  I personally find "Available Commands:"  a poor title in this case as it will surely cause confusion compared to the group names.  How about we call the section "Additional commands:" and put it after all the groups?

Ok, I can also add this, but I think we should keep the exiting behavior if the user defines no group at all. That is, if no group is defined, the "group title" of a command without a Group set is "Available Commands" and if at least one group is defined, it would be "Additional Commands".

Good point. This is actually important to keep the help the same for existing programs 👍

Also, if users wants to see "Additional Commands", they can also get this behavior by defining a new group titled "Additional Commands" and put these commands into that group.
So, do we really want this?

The "Additional Commands" automatic group is just to catch commands that were not put into any group. It should not actually happen but experience shows we better play it safe. It may happen fire the help and completion command where devs may forget to give them a group or not realize they can.

If it's complicated to implement, leave it and we can see on the next review.

4. The ordering of the Groups in the help output is not obvious to a user, especially when implicitly creating groups.  We would at least need to document it.  But let's see if we keep implicit creation or not (see in-line comments).

After removing the implicit definition, the order will be the order of the AddGroup commands. I'll add this in the manual.

Perfect.

Looking forward to the next iteration! 🎉

@aawsome aawsome force-pushed the add-groups branch 2 times, most recently from 5ed67cb to 21ffa2d Compare October 6, 2022 06:54
@aawsome
Copy link
Contributor Author

aawsome commented Oct 6, 2022

I rebased this PR to the current master and made the mentioned changes.
I now get a linting error Error: struct of size 720 bytes could be of size 712 bytes (maligned) which I don't know how to solve. And IMO I will add a test of SetCompletionCommandGroup.

When these two points (and squashing the commits) are tackled, I think this PR will be ready.

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small suggestions to cleanup a bit.

I haven't tested yet.

command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
@marckhouzam
Copy link
Collaborator

I rebased this PR to the current master and made the mentioned changes.
I now get a linting error Error: struct of size 720 bytes could be of size 712 bytes (maligned) which I don't know how to solve.

If you move the new completionCommandGroup declaration slightly higher right under the declaration of helpCommandGroup, the linter error goes away. It has to do with memory alignment, although I haven't looked exactly at what was not well aligned.

And IMO I will add a test of SetCompletionCommandGroup.

Yes please.

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I like this a lot. Nice job @aawsome. I put a bunch of minor suggestions about improving comments and such, but you should just be able to accept them in Github which should be very easy.

I believe the only thing remaining is when a command is not part of a group. I have created the "Administration Commands:" and "System Commands:" groups but "forgot" to assign the login command to any group.
Currently we see this:

My CLI

Usage:
  prog [command]

Available Commands:
  login             Login to the platform

System Commands:
  config            Configuration for the CLI
  init              Initialize the CLI
  update            Update the CLI
  version           Version information

Administration Commands:
  completion        Generate the autocompletion script for the specified shell
  help              Help about any command

Flags:
  -h, --help   help for prog

I think the section "Available Commands:" looks bad.
However, I would personally be ok with leaving it as is and saying that it is up to developers to properly define their groups and not leave commands without a group.

@jpmcb Thoughts?

@aawsome I'm finished with the review. Whenever you find time, if you can update, and then we'll be pretty much ready to merge 🎉 .

@aawsome aawsome force-pushed the add-groups branch 3 times, most recently from 19fc3af to 1f716e7 Compare October 8, 2022 21:17
aawsome and others added 2 commits October 8, 2022 23:23
Co-authored-by: Marc Khouzam <marc.khouzam@gmail.com>
@aawsome
Copy link
Contributor Author

aawsome commented Oct 8, 2022

Thanks a lot for your review, @marckhouzam
I resolved all comments, added a test for the completion command and rebased this PR.
I hope now everything is fine 😉

Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would personally be ok with leaving it as is and saying that it is up to developers to properly define their groups and not leave commands without a group.

I think this is should be the desired behavior since it essentially makes this feature backwards compatible: if someone doesn't define any groups, I'd expect this the help section to continue to work like it does today.


I've pondered the design decisions this PR carries and while I think this implementation is good, I wonder if this would be due for a refactor down the road since people may want a way to do more interesting things with groups beyond matching on strings (i.e., a group struct with a superset of cobra commands). But we'd need to see if people would even want that.

TLDR: I think this is good for now and we can merge it to get this behavior in

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
@marckhouzam
Copy link
Collaborator

I tested the latest version and it looked great.
But I kept being bothered by the "Additional commands:" section, mostly when it was empty.
So I went ahead and pushed a new commit that fixed it. This is the result.

If no groups are defined at all we remain backwards-compatible and get:

$ prog -h
My CLI

Usage:
  prog [command]

Available Commands:
  completion        Generate the autocompletion script for the specified shell
  config            Configuration for the CLI
  help              Help about any command
  version           Version information

Flags:
  -h, --help   help for prog

Use "prog [command] --help" for more information about a command.

If all subcommands have a group, we no longer get an empty "Available Commands:" line:

$ prog -h
My CLI

Usage:
  prog [command]

System
  config            Configuration for the CLI
  version           Version information

Administration
  completion        Generate the autocompletion script for the specified shell
  help              Help about any command

Flags:
  -h, --help   help for prog

Use "prog [command] --help" for more information about a command.

And if some commands have groups but some do not we get a new "Additional Commands:" section at the bottom:

$ prog -h
My CLI

Usage:
  prog [command]

System
  config            Configuration for the CLI

Administration
  version           Version information

Additional Commands:
  completion        Generate the autocompletion script for the specified shell
  help              Help about any command

Flags:
  -h, --help   help for prog

Use "prog [command] --help" for more information about a command.

@aawsome @jpmcb do you agree this is better? If not, we can just remove the top commit that I pushed.
If you like it, I'm ready to merge.

I wonder if this would be due for a refactor down the road since people may want a way to do more
interesting things with groups beyond matching on strings (i.e., a group struct with a superset of cobra commands).
But we'd need to see if people would even want that.

That is interesting and may indeed have potential for improvement. But I also can't tell at the moment what that would look like and if it will be in demand. So I agree with @jpmcb that we should go ahead with this version, which we know people want, and see where it takes us.

@marckhouzam marckhouzam added area/cobra-command Core `cobra.Command` implementations lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready and removed triage/needs-info Needs more investigation from maintainers or more info from the issue provider labels Oct 9, 2022
@aawsome
Copy link
Contributor Author

aawsome commented Oct 9, 2022

So I went ahead and pushed a new commit that fixed it.

Just had a look at your commit. This is how I would have added these functionalities. So,your changes look good for me!

jpmcb
jpmcb approved these changes Oct 10, 2022
Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @marckhouzam!!

@jpmcb jpmcb merged commit 2169adb into spf13:main Oct 10, 2022
16 checks passed
@Airblader
Copy link

I'd just like to say thank you from a backseat watcher and user of the library to @aawsome for bringing this over the finish line even after such a long time! And of course everyone else who helped along the way.

@marckhouzam
Copy link
Collaborator

Yes thank you @aawsome ! You have shown great professionalism in what can be considered a frustrating situation 🙂

The community will reap the rewards 🎉

@Airblader
Copy link

Airblader commented Oct 12, 2022

If you don't mind, a follow-up question while adopting this: the groups appear in the order they are defined, but what about the commands in the group? Can we order them? It seems that currently this is alphabetical, which isn't usually very meaningful.

Edit: Just found

// EnableCommandSorting controls sorting of the slice of commands, which is turned on by default.
// To disable sorting, set it to false.
var EnableCommandSorting = defaultCommandSorting

so

cobra.EnableCommandSorting = false

does the trick. 👍

hoshsadiq pushed a commit to zulucmd/zulu that referenced this pull request Dec 31, 2022
Adds the possibility to group the commands in the help message.

Merge spf13/cobra#1003

closes #836
closes #1271
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cobra-command Core `cobra.Command` implementations kind/feature A feature request for cobra; new or enhanced behavior lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subcommand categories ? Add ability to group commands in help message