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

Check for group presence after full initialization #1839

Merged
merged 1 commit into from Oct 24, 2022

Conversation

marckhouzam
Copy link
Collaborator

Fixes #1831
Replaces #1835

By moving the check for help group existence to "ExecuteC()" we no longer need groups to be added before AddCommand() is called. This provides more flexibility to developers and works better with the use of "init()" for command creation.

cc @aawsome

@marckhouzam marckhouzam added the area/cobra-command Core `cobra.Command` implementations label Oct 23, 2022
@github-actions github-actions bot removed the area/cobra-command Core `cobra.Command` implementations label Oct 23, 2022
Copy link
Contributor

@aawsome aawsome left a comment

Choose a reason for hiding this comment

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

Very good idea @marckhouzam to put this check in the execute branch!

I've got only nit-picking comments; -)

command.go Show resolved Hide resolved
command_test.go Outdated Show resolved Hide resolved
Fixes spf13#1831

By moving the check for help group existence to "ExecuteC()" we no
longer need groups to be added before AddCommand() is called.  This
provides more flexibility to developers and works better with the use
of "init()" for command creation.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
jpmcb
jpmcb approved these changes Oct 24, 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.

This looks great to me. Maybe we can cut a quick 1.6.1 to push this out since it was unintended behavior we'd like to fix?

@marckhouzam marckhouzam merged commit 10cf7be into spf13:main Oct 24, 2022
16 checks passed
@marckhouzam marckhouzam deleted the fix/noOrderForGroups branch October 24, 2022 15:12
@marckhouzam
Copy link
Collaborator Author

This looks great to me. Maybe we can cut a quick 1.6.1 to push this out since it was unintended behavior we'd like to fix?

If you're up for it I think it would improve the user experience for this popular feature 👍

jpmcb pushed a commit to jpmcb/cobra that referenced this pull request Oct 24, 2022
Fixes spf13#1831

By moving the check for help group existence to "ExecuteC()" we no
longer need groups to be added before AddCommand() is called.  This
provides more flexibility to developers and works better with the use
of "init()" for command creation.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
jpmcb added a commit that referenced this pull request Oct 24, 2022
Fixes #1831

By moving the check for help group existence to "ExecuteC()" we no
longer need groups to be added before AddCommand() is called.  This
provides more flexibility to developers and works better with the use
of "init()" for command creation.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
Co-authored-by: Marc Khouzam <marc.khouzam@gmail.com>
@umarcor
Copy link
Contributor

umarcor commented Nov 12, 2022

@marckhouzam please milestone this!

@marckhouzam marckhouzam added this to the 1.6.1 milestone Nov 12, 2022
@marckhouzam
Copy link
Collaborator Author

@marckhouzam please milestone this!

Thanks! I created a 1.6.1 milestone for this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subcommand Group | panic: Group id 'Group1' is not defined for subcommand 'app1 sub1'
4 participants