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.

Sign up for GitHub

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

Jump to bottom

Add remove group method for command #1997

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add remove group method for command #1997

wants to merge 9 commits into from

Conversation

JunNishimura
Copy link
Contributor

@JunNishimura JunNishimura commented Jul 16, 2023 •
edited
Loading

Overview

This PR is about #1911.
Sorry, I missed that a PR(#1912) has already been submitted regarding this Issue.
However, it appears that a PR has not been updated, so I am submitting this just in case.

I added RemoveGroup method for command. RemoveGroup method removes groups from commandgroups and at the same time initializes the command's groupID.

Related Issue

Related PR

@JunNishimura JunNishimura mentioned this pull request Jul 16, 2023
Open
@jpmcb jpmcb added area/cobra-command Core `cobra.Command` implementations kind/feature A feature request for cobra; new or enhanced behavior labels Jul 22, 2023
jpmcb
jpmcb approved these changes Jul 22, 2023
View reviewed changes
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.

This makes sense to me and looks to be on par with how RemoveCommand works 👍🏼

command_test.go
@@ -1862,6 +1862,61 @@ func TestAddGroup(t *testing.T) {
checkStringContains(t, output, "\nTest group\n cmd")
}

func TestRemoveSingleGroup(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding tests! Definitely makes it easier to validate!!

@jpmcb jpmcb added the lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready label Jul 22, 2023
@valllabh valllabh mentioned this pull request Jul 23, 2023
Closed
@JunNishimura
Copy link
Contributor Author

@jpmcb
Thanks for checking the PR!!

One question, do I need to ask one more person to review this PR to merge it into the main branch?

marckhouzam
marckhouzam reviewed Jul 26, 2023
View reviewed changes
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.

A thought I had. If the user calls remove group with an invalid group ID, the current logic will simply ignore it. Is that what we want or do you prefer to help the developer notice their mistake?

site/content/user_guide.md Outdated Show resolved Hide resolved
command.go Outdated
@@ -1311,6 +1311,30 @@ func (c *Command) AddGroup(groups ...*Group) {
c.commandgroups = append(c.commandgroups, groups...)
}

// RemoveGroup removes one or more command groups to this parent command.
func (c *Command) RemoveGroup(groupIDs ...string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i’m not sure, but I don’t think this will allow removing a group for the help or completion commands. Do we want to support removing groups for those two commands?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting use case. I guess in theory someone might want to remove those from a group that have been added. But they are also sort of the "default" commands you get. What do you think Marc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, I would like cobra to give users as much freedom of development as possible. It is up to the developers to decide whether they want to keep the help and completion commands as existing groupies or remove them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree and I'm 100% for freedom of use. It more just becomes a matter of not breaking long standing existing features. But this is new functionality anyways. So I'm good with it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed as follows. If the group id passed as argument match, the group id of the help and completion command is reset in the RemoveGroup method. I would appreciate it if you check the code again.

@github-actions github-actions bot removed the area/cobra-command Core `cobra.Command` implementations label Jul 27, 2023
@JunNishimura
Copy link
Contributor Author

@marckhouzam @jpmcb
Sorry it took so long to reply.

Is that what we want or do you prefer to help the developer notice their mistake?

I thought it would be better to output an error if the expected group does not disappear as a result of calling the RemoveGroup command with invalid IDs, since that is not the result expected by the developer. Therefore I fixed the RemoveGroup command to return an error.

What do you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpmcb jpmcb jpmcb approved these changes

@marckhouzam marckhouzam marckhouzam left review comments

@valllabh valllabh valllabh approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JunNishimura @marckhouzam @valllabh @jpmcb