add MarkIfFlagPresentThenOthersRequired #2200

faizan-siddiqui · 2024-10-23T05:59:31Z

This PR introduces a new flag group function, MarkIfFlagPresentThenOthersRequired, which enforces a "one-way required together" relationship among flags. This means that if a primary flag is set, then other dependent flags must also be set. This allows users to make certain flags conditionally required based on the presence of another flag, while maintaining flexibility when the primary flag is not provided.

Example

Consider a command called get-range that uses the flags --start and --end to specify a date range. By default, the command can run without either flag, using default start and end values (e.g., start=someDefaultStart and end=Now). However, you may want to enforce that if the user specifies an --end, they must also specify a --start.

cmd := &cobra.Command{
    Use: "get-range",
}

cmd.Flags().String("start", "", "Start date")
cmd.Flags().String("end", "", "End date")

// If --end is set, --start is required
cmd.MarkIfFlagPresentThenRequired("end", "start")

If the command is called with only --end=value, it will trigger an error because --start is missing.
If called without any flags or only with --start, it will run using the default values or the provided start date.

This change makes it easier to enforce dependencies between flags while allowing flexible defaults when the primary flag is not specified.

CLAassistant · 2024-10-23T05:59:36Z

All committers have signed the CLA.

faizan-siddiqui · 2024-10-23T06:04:23Z

@marckhouzam Would love to get this in thanks! Let me know if you have any questions :)

faizan-siddiqui · 2024-11-13T03:01:24Z

@dpetersen @markbates @EdwardBetts Would love to get this in thanks! Let me know if you have any questions :)

danpalmer · 2025-01-06T22:03:02Z

@marckhouzam is there anything we can do to get this moving? Alternatively we're happy to have discussions about it being out of scope, it just felt fairly obviously useful to us.

marckhouzam · 2025-01-07T11:54:18Z

I like the idea. I’ll put it on my queue to review. Thank you.

flag_groups.go

flag_groups_test.go

flag_groups.go

Co-authored-by: ccoVeille <[email protected]>

…ond flag Co-authored-by: ccoVeille <[email protected]>

faizan-siddiqui · 2025-01-16T02:26:25Z

Hi @ccoVeille thank you for the review, I have committed your suggested changes

flag_groups.go

ccoVeille · 2025-01-16T07:42:46Z

flag_groups.go

+	requiredAsGroupAnnotation             = "cobra_annotation_required_if_others_set"
+	oneRequiredAnnotation                 = "cobra_annotation_one_required"
+	mutuallyExclusiveAnnotation           = "cobra_annotation_mutually_exclusive"
+	ifPresentThenOthersRequiredAnnotation = "cobra_annotation_if_present_then_others_required"


Could you rename them in a way they share a common prefix, not suffix

These constants are defined at the package level, so IDE will suggest them in any method when coding in that package

I would like to see something like this

Suggested change

requiredAsGroupAnnotation = "cobra_annotation_required_if_others_set"

oneRequiredAnnotation = "cobra_annotation_one_required"

mutuallyExclusiveAnnotation = "cobra_annotation_mutually_exclusive"

ifPresentThenOthersRequiredAnnotation = "cobra_annotation_if_present_then_others_required"

annotationGroupRequired = "cobra_annotation_required_if_others_set"

annotationRequiredOne = "cobra_annotation_one_required"

annotationMutuallyExclusive = "cobra_annotation_mutually_exclusive"

annotationGroupDependent = "cobra_annotation_if_present_then_others_required"

The names of the constants are just suggestions to give you an idea

github-actions · 2025-02-18T05:18:11Z

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 · 2025-02-18T05:18:33Z

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.

faizan-siddiqui · 2025-02-18T05:20:16Z

Thanks @ccoVeille I have addressed your comment and added an explicit test case for ensuring the annotation is appended as expected and is guarded against future refactorings, please review if you can :)

ccoVeille

Thanks for working on the changes I suggested.

Let's @marckhouzam review everything now

faizan-siddiqui · 2025-03-04T03:46:12Z

Thanks @ccoVeille for helping out with the review and suggested changes
@marckhouzam would love to get this in, please let me know if any issues

faizan-siddiqui · 2025-03-23T06:58:09Z

Hi @marckhouzam , @ccoVeille , is there anything else you may need to have this merged in? Let me know if there are any questions.

ccoVeille

Hi @faizan-siddiqui

The PR is OK for me in term of code. I just reviewed again, and I made a few minor feedbacks.

Please note, while I feel confident in reviewing the code, I don't find myself n
legitimate to say whether these changes, and so features additions, are fine in the context of cobra,it's design, philosophy, and history.

So here, you will have to wait for @marckhouzam to review your changes.

ccoVeille · 2025-03-25T09:55:41Z

flag_groups.go

+// MarkIfFlagPresentThenOthersRequired marks the given flags so that if the first flag is set,
+// all the other flags become required.
+func (c *Command) MarkIfFlagPresentThenOthersRequired(flagNames ...string) {
+	if len(flagNames) < 2 {
+		panic("MarkIfFlagPresentThenRequired requires at least two flags")
+	}


🎨 Nitpicking

I'm unsure about this wording here 🤔

Shouldn't it be, something like this

Suggested change

// MarkIfFlagPresentThenOthersRequired marks the given flags so that if the first flag is set,

// all the other flags become required.

func (c *Command) MarkIfFlagPresentThenOthersRequired(flagNames ...string) {

if len(flagNames) < 2 {

panic("MarkIfFlagPresentThenRequired requires at least two flags")

}

// MarkIfFlagPresentThenOthersRequired marks the given flags so that if the first flag is set,

// all the other flags become required.

func (c *Command) MarkIfFlagPresentThenOthersRequired(flagNames ...string) {

if len(flagNames) < 2 {

panic("MarkIfFlagPresentThenOthersRequired requires at least two flags")

}

ccoVeille · 2025-03-25T09:56:49Z

flag_groups.go

+	for _, v := range flagNames {
+		f := c.Flags().Lookup(v)
+		if f == nil {
+			panic(fmt.Sprintf("Failed to find flag %q and mark it as being in an if present then others required flag group", v))


Shouldn't you mention the method name as you do in the other panic?

Or maybe, both shouldn't mention it

ccoVeille · 2025-03-25T09:58:01Z

flag_groups.go

+	for flagList, flagnameAndStatus := range data {
+		flags := strings.Split(flagList, " ")
+		primaryFlag := flags[0]
+		remainingFlags := flags[1:]


Are we sure this won't panic if the flags is limited to one element?

ccoVeille · 2025-03-25T09:58:37Z

flag_groups.go

+	for flagList, flagnameAndStatus := range ifPresentThenRequiredGroupStatus {
+		flags := strings.Split(flagList, " ")
+		primaryFlag := flags[0]
+		remainingFlags := flags[1:]


Same question about flags length being 1 or 0

ccoVeille · 2025-03-25T10:00:43Z

flag_groups.go

+func validateIfPresentThenRequiredFlagGroups(data map[string]map[string]bool) error {
+	for flagList, flagnameAndStatus := range data {
+		flags := strings.Split(flagList, " ")
+		primaryFlag := flags[0]


Split spec explains it can return nil, so here flags[0] could panic

Maybe it's not possible, maybe it's already checked somewhere else before reaching this line of code, but I'm always suspicious when I see such optimistic code

add MarkIfFlagPresentThenOthersRequired

ca3972e

added test cases

4944350

update test

23cadf7

ccoVeille reviewed Jan 8, 2025

View reviewed changes

flag_groups.go Outdated Show resolved Hide resolved

flag_groups_test.go Outdated Show resolved Hide resolved

flag_groups.go Outdated Show resolved Hide resolved

faizan-siddiqui and others added 3 commits January 16, 2025 13:24

update error message to be more readable

ad5e8c3

Co-authored-by: ccoVeille <[email protected]>

add more verbose test cases

cb45935

Co-authored-by: ccoVeille <[email protected]>

fix comment to say other flags will be marked required instead of sec…

1c44c42

…ond flag Co-authored-by: ccoVeille <[email protected]>

faizan-siddiqui requested a review from ccoVeille January 16, 2025 02:26

faizan-siddiqui added 2 commits January 16, 2025 02:39

fix test error message

dd7bc42

Merge branch 'spf13:main' into patch-1

a9bed3a

ccoVeille reviewed Jan 16, 2025

View reviewed changes

add explicit test for testing annotation

578b074

Merge branch 'spf13:main' into patch-1

8d0e71e

faizan-siddiqui requested a review from ccoVeille February 18, 2025 05:20

ccoVeille approved these changes Feb 18, 2025

View reviewed changes

ccoVeille reviewed Mar 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add MarkIfFlagPresentThenOthersRequired #2200

add MarkIfFlagPresentThenOthersRequired #2200

faizan-siddiqui commented Oct 23, 2024

CLAassistant commented Oct 23, 2024 •

edited

Loading

faizan-siddiqui commented Oct 23, 2024

faizan-siddiqui commented Nov 13, 2024

danpalmer commented Jan 6, 2025

marckhouzam commented Jan 7, 2025

faizan-siddiqui commented Jan 16, 2025

ccoVeille Jan 16, 2025

github-actions bot commented Feb 18, 2025

github-actions bot commented Feb 18, 2025

faizan-siddiqui commented Feb 18, 2025

ccoVeille left a comment

faizan-siddiqui commented Mar 4, 2025

faizan-siddiqui commented Mar 23, 2025

ccoVeille left a comment

ccoVeille Mar 25, 2025

ccoVeille Mar 25, 2025

ccoVeille Mar 25, 2025

ccoVeille Mar 25, 2025

ccoVeille Mar 25, 2025

add MarkIfFlagPresentThenOthersRequired #2200

Are you sure you want to change the base?

add MarkIfFlagPresentThenOthersRequired #2200

Conversation

faizan-siddiqui commented Oct 23, 2024

Example

CLAassistant commented Oct 23, 2024 • edited Loading

faizan-siddiqui commented Oct 23, 2024

faizan-siddiqui commented Nov 13, 2024

danpalmer commented Jan 6, 2025

marckhouzam commented Jan 7, 2025

faizan-siddiqui commented Jan 16, 2025

ccoVeille Jan 16, 2025

Choose a reason for hiding this comment

github-actions bot commented Feb 18, 2025

github-actions bot commented Feb 18, 2025

faizan-siddiqui commented Feb 18, 2025

ccoVeille left a comment

Choose a reason for hiding this comment

faizan-siddiqui commented Mar 4, 2025

faizan-siddiqui commented Mar 23, 2025

ccoVeille left a comment

Choose a reason for hiding this comment

ccoVeille Mar 25, 2025

Choose a reason for hiding this comment

ccoVeille Mar 25, 2025

Choose a reason for hiding this comment

ccoVeille Mar 25, 2025

Choose a reason for hiding this comment

ccoVeille Mar 25, 2025

Choose a reason for hiding this comment

ccoVeille Mar 25, 2025

Choose a reason for hiding this comment

CLAassistant commented Oct 23, 2024 •

edited

Loading