-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Make Groups public field for command #1835
Conversation
Thanks @andig . Also could you describe how users would make use of this change to improve the use of groups? |
@marckhouzam the feature has only been introduced 9 days ago and like does not have wide-spread use. It also encourages a close coupling. Happy to rework, but it seems better to make this change cleanly and quickly. |
I'll let @jpmcb disagree, but in my opinion, once it is out, that's it, projects can be using it. That is why I was so detailed on getting as good an API as we could from the start. |
I'm not seeing how this is a breaking change? Looks like we want to add the I'm also not seeing how this solves #1831. Can you provide an example using this newly exposed API from this patch to give an explanation / example? |
@jpmcb 7aa995f is a breaking change (
The problem with #1831 is that the root commands groups must be
Imho the current
Update even moving sub command declaration to it's |
@marckhouzam I do by no means want to imply you did a bad job. Hope it doesn't come across like that. I may really be missing something! Just didn't find a good approach that fits (my) current pattern of using cobra commands. |
Not at all! I was just explaining why we were so demanding with the original review. Once an API is published we can never break it 😅 And thanks for continuing to improve this feature. Clearly people want it as we already see multiple users 👍 |
I'd like to think a bit about this |
Let's look at the basics of the situation.
These calls must be in the above order because (and only because) we verify if all groups properly exist when calling The problem is that when using Notice that if we don't verify the correct existence of groups, then the ordering does not matter and everything will work. But as discussed in the original PR, it would become easy for a typo in a group id to go unnoticed which would make the subcommand with the wrong id not to be shown in the help. So instead of completely removing the check, how about we move it to a point where the order of initialization no longer matters? I believe we can do that by adding the check in What do you think? |
I had actually thought about that but kinda forgot. Sounds very good- validate existence of all group/IDs without depending on order. |
I think that's a very solid solution here: Let's not re-work the API, let's decouple it so calls don't depend on ordering. |
@marckhouzam would you be so kind to provide the follow-up PR? I'm unsure where to put the validation now. |
I've created #1839. If this solution looks good, we can close the current PR. |
Fix #1831.
This PR is a BC break as it replaces
Groups()
with theGroups
field. This could be fixed by keepingCommandGroups
as public member name but felt clearer not duplicating the api.