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

v3 MutuallyExclusive flags are not included in help text #1858

Closed
3 tasks done
joshfrench opened this issue Jan 15, 2024 · 7 comments · Fixed by #1863
Closed
3 tasks done

v3 MutuallyExclusive flags are not included in help text #1858

joshfrench opened this issue Jan 15, 2024 · 7 comments · Fixed by #1863
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this

Comments

@joshfrench
Copy link
Contributor

joshfrench commented Jan 15, 2024

Checklist

  • Are you running the latest v3 release? The list of releases is here.
  • Did you check the manual for your release? The v3 manual is here.
  • Did you perform a search about this feature? Here's the GitHub guide about searching.

Hi hi! I recognize v3 is alpha software, so my question is more "is this implemented yet or am I doing it wrong?" Assuming it just hasn't been worked on yet, if someone can point me to where I might start investigating I'll see about a PR :)

package main

import (
    "context"
    "os"

    "github.com/urfave/cli/v3"
)

func main() {
    cmd := &cli.Command{
        Name: "test",
        Flags: []cli.Flag{
            &cli.StringFlag{
                Name:  "s",
                Usage: "this works",
            },
        },
        MutuallyExclusiveFlags: []cli.MutuallyExclusiveFlags{{
            Flags: [][]cli.Flag{
                {
                    &cli.BoolFlag{
                        Name:  "b",
                        Usage: "no display :(",
                    },
                },
            },
        },
        },
    }

    cmd.Run(context.Background(), os.Args)
}
% test -h
NAME:
   test - A new cli application

USAGE:
   test [global options] [command [command options]] [arguments...]

COMMANDS:
   help, h  Shows a list of commands or help for one command

GLOBAL OPTIONS:
   -s value    this works
   --help, -h  show help (default: false)

The same holds true for subcommands as well.

@joshfrench joshfrench added area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this labels Jan 15, 2024
@dearchap
Copy link
Contributor

@joshfrench This feature hasnt been implemented yet. You are welcome to submit a PR. The changes would be primarily in the template.go file which has the template to render help text.

@joshfrench
Copy link
Contributor Author

joshfrench commented Feb 3, 2024

Just getting around to this...

The basic case is straightforward, but once you account for flag categories I can see at least two approaches. The first doesn't discriminate between mutex and regular flags:

cmd := &cli.Command{
        Name: "test",
        MutuallyExclusiveFlags: []cli.MutuallyExclusiveFlags{{
            Flags: [][]cli.Flag{
                {
                    &cli.BoolFlag{
                        Name:  "b1",
                        Category: "one",
                    },
                },
                {
                    &cli.BoolFlag{
                        Name:  "b2",
                        Category: "two",
                    },
                },
            },
        },
        },
    }
GLOBAL OPTIONS:
   one

   --b1

  two

  --b2

This is flexible but doesn't quite track with my mental model of how mutex flags are meant to be used. The second approach would treat each mutex group as its own implicit category:

cmd := &cli.Command{
        Name: "test",
        Flags: []cli.Flag{
            &cli.StringFlag{
                    Name: "s1",
                    Category: "one",
            }
        },
        MutuallyExclusiveFlags: []cli.MutuallyExclusiveFlags{{
            Flags: [][]cli.Flag{
                {
                    &cli.BoolFlag{
                        Name:  "b1",
                    },
                },
                {
                    &cli.BoolFlag{
                        Name:  "b1",
                    },
                },
            },
        },
        },
    }
GLOBAL OPTIONS:
   one

   --s1

  ???

  --b1 | --b2

This approach is more prescriptive, but is closer to how I would expect things to work. Additionally:

  • It suggests each flag group might benefit from a Name field.
  • It does not address what happens if a mutex flag also sets a category, or if/how a user would be prevented from doing so.
  • The exact formatting of mutex flags needs some thought; with sibling groups and default values it might become unwieldy: --string1 value | [ --bool1 (default: true) | --bool2 (default: false) ] | ...

Thoughts before I dive into category handling? I think I'm inclined to go with option 1 and leave it up to the user to categorize mutex flags how they wish:

cmd := &cli.Command{
        Name: "test",
        MutuallyExclusiveFlags: []cli.MutuallyExclusiveFlags{{
            Flags: [][]cli.Flag{
                {
                    &cli.BoolFlag{
                        Name:  "b1",
                        Category: "one",
                    },
                },
                {
                    &cli.BoolFlag{
                        Name:  "b2",
                        Category: "one",
                    },
                    &cli.BoolFlag{
                        Name:  "b3",
                        Category: "i know what i'm doing",
                    },
                },
            },
        },
        },
    }

@dearchap
Copy link
Contributor

dearchap commented Feb 4, 2024

@joshfrench Yes option 1 would be good to provide maximum flexibility. But also we can have a check that all the mutex flags should be in the same category. It doesnt make sense to have these flag groups across multiple categories.

@joshfrench
Copy link
Contributor Author

@dearchap Is there a better place for that check than the MutuallyExclusiveFlags.check method? That seems aimed at catching user error, not cli developer error.

@joshfrench
Copy link
Contributor Author

joshfrench commented Feb 4, 2024

Some alternatives to treating it as an error:

  • Just coerce all the flags in a group to the first (or last?) category discovered within that group.
  • Add a Category field to the group; ignore any Category set on flags within the group.

The latter strikes me as both easiest to implement and to understand as an end-user.

@dearchap
Copy link
Contributor

dearchap commented Feb 4, 2024

Yes I think the latter approach of adding a Category field to the group is better.

@joshfrench
Copy link
Contributor Author

PR updated! I did add a SetCategory() method to allow mutex groups to propagate their category downward to the flags within the group, which is triggering a docs warning -- I'll wait for a review before doing anything else. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants