refactor(flag_groups): flag groups implementation improved

[evermake]

Hello, cobra community! This PR size is not small, but I believe it will be considered, because I tried my best to improve quality of the code and implement new flag groups, because I've found this feature important for me and other users.

This PR changes the flag groups feature logic. New implementation is more clean, readable and extendable (hope it won't be just my opinion). Updated logic makes adding new groups a trivial task.

Important: this PR conflicts with PRs #1768 and #1747, because these PRs adds new groups support by extending current implementation of the flag groups, while this PR changes current implementation (and makes it simpler).

This PR does not close any issues, however, after changing the logic I've also easily implemented new groups to close issues #1739, #1738, #1537, #1216. I can add test cases for these new groups and add new small PRs to close mentioned issues after this PR will be merged. I have not included these changes here to avoid increasing the PR even more.

The following changes have been made:

1. Main change: flags annotating by "cobra_annotation_required_if_others_set" and "cobra_annotation_mutually_exclusive" annotations was removed as well as all related and hard-to-understand "hacks" to combine flags back into groups on validation process. Instead, flagGroups field was added to the Command struct. flagGroups field is a list of the (new) structs flagGroup, which represents the "relationships" between flags within the command. Now validateFlagGroups function just iterates over command's flag groups and runs validation for each group.

2. "Required together" and "mutually exclusive" groups logic was updated by implementing a new flagGroup interface in requiredTogetherFlagGroup and mutuallyExclusiveFlagGroup structs.

3. enforceFlagGroupsForCompletion Command's method was renamed to adjustByFlagGroupsForCompletions.

4. Groups failed validation error messages were changed:

• "if any flags in the group [...] are set they must all be set; missing [...]" to "flags [...] must be set together, but [...] were not set"
• "if any flags in the group [...] are set none of the others can be; [...] were all set" to "exactly one of the flags [...] can be set, but [...] were set"

5. Not found flag error messages were updated from "Failed to find flag %q and mark it as being required in a flag group" and "Failed to find flag %q and mark it as being in a mutually exclusive flag group" to the common "flag %q is not defined".

6. TestValidateFlagGroups test was updated in flag_groups_test.go.

• getCmd function was updated and test flag names were changed to improve readability
• 2 testcases (Validation of required groups occurs on groups in sorted order and Validation of exclusive groups occurs on groups in sorted order) were removed, because groups validation now occur in the same order those groups were registered, not in the sorted order
• other 16 testcases are preserved with updated descriptions and error messages

✅ The completions generation tests that contain flag groups related testcases and updated flag groups tests, as well as all other tests, have been passed.

✅ API was not changed: MarkFlagsRequiredTogether and MarkFlagsMutuallyExclusive functions have the same signatures.

I will be happy to know the opinion of maintainers and other contributors and I'm ready to edit/fix this PR, if something is not good.

[CLAassistant]

All committers have signed the CLA.

[github-actions[bot]]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[umarcor]

/cc @johnSchnake.

[github-actions[bot]]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[evermake]

Some updates on this? I'm ready to improve this PR, if necessary, and work on flag groups further.

[plastikfan]

Hi @evermake, although I havent looked in detail at your code yet, on the face of it, I like your idea of simplification of the whole feature. My approach when I submitted PR #1768, was only to add the minimum amount of code to get the new functionality, rather than simplifying the whole feature. This was mainly due to that automated github-actions bot which posts the message "This PR exceeds the recommended size of 200 lines." that I think does more damage to new contributors than good. I was put off by this, resulting in me not making the better decision to redesign the whole feature as you appear to have done. Also, what held me up was the test cases for completions scenario, which still puzzles me as to how this works, hence me not having finalised my PR. If you have a redesign in mind, then I don't mind not submitting my PR.

[plastikfan]

Hi again @evermake, have you implemented 1 way relationships between flags, which was not in the currently implementation. In my PR #1768 (for issue #1739), you could define that if flag A is present then Flag B must be present, but not the other way around? (ie the MarkFlagsDependsOn and MarkFlagDependsOnAny validation groups), otherwise this PR is just a refactoring excercise without introducing new functionality.

[evermake]

Hello, @plastikfan! I implemented "MarkFlagDependentOn" locally (just to check result) and removed it, as I mentioned in the PR description: "I have not included these changes here to avoid increasing the PR even more". Implementing such feature after the redesign takes a few minutes, the main difficulty — writing tests.

By the way, I don't fully understand how does completions generation work either :) Could someone explain it or maybe there is a documentation somewhere?

[plastikfan]

Hi @evermake, thanks for that. Yeah I now realise the intent of your PR. So adding the new functionality for the new 1 way validation groups becomes a trivial matter and I whole-heartedly support your approach.

As regarding the completions, there is a document zsh_completions.md, that may help. Perhaps my understanding of the completions was difficult because of the old implementation of the group functionality. Maybe your new design, will make writing the complations tests a bit easier.

[marckhouzam]

Hi @evermakeand thanks for the efforts. I haven't had time to look at the PR but for completions you can look at shell_completions.md for user documentation. As for the completion implementation it is in completions.go and you can find examples of tests in completions_test.go.

Oh and the PR seems to need a rebase. Thanks again and we appreciate your patience.

[evermake]

@marckhouzam , yeah, PR needs to updated, I'll rebase soon and check everything once again. Also I'm going to implement requested groups (dependent flags #1739 and required mutually exclusive #1761) in my fork and then right after this PR will be merged I will create new PR's to close the mentioned issues. Right?

By the way, though all tests are passed after my changes, I want someone to check my code, someone who understands the overall structure of the cobra and how was flag groups implemented before — I hope I didn't break some functionality that depended on current implementation.

[github-actions[bot]]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[github-actions[bot]]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[evermake]

Rebased.

[github-actions[bot]]

The Cobra project currently lacks enough contributors to adequately respond to all PRs. This bot triages issues and PRs according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied. - After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied and the PR is closed. You can:
• Make a comment to remove the stale label and show your support. The 60 days reset. - If a PR has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interested in reopening.

[github-actions[bot]]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

[evermake]

Should I continue to work on this? It's been a long time since I created a PR, but I'm ready to dive into this again and finish work, but I wish to know whether the PR will be reviewed and merged.

[marckhouzam]

The maintainers have had very little time to allocate to Cobra recently. So I cannot make any promises. However, if you keep the PR active, I hope that one of us will be able to have a look at it.

[travisjeffery]

Hope you get this in, being able to do stuff like https://github.com/spf13/cobra/issues/1537 would be great.

[AmadeusK525]

Commenting to get some activity on the PR. I looked at the code briefly and it seems pretty sturdy to me, this would be greatly appreciated and it would help a lot in implementing new features regarding the flag groups (the one that I - and most devs, probably - would like the most is the ability to group flags for the usage message display). Are any of the maintainers able to check this out?