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

Show the actual available flags in useline #2105

Closed
wants to merge 1 commit into from

Conversation

lugray
Copy link

@lugray lugray commented Feb 1, 2024

Use common useline notation for available flags:

  • [-f foo] for (standard) optional flags
  • [-f foo | -b bar] for mutually exclusive flags
  • -f foo for required flags
  • {-f foo | -b bar} for mutually exclusive flags where one is required
  • [-f foo -b bar] for flags required as a group

For a boolean flag f, foo is dropped from the above. The foo/bar comes from flag.UnquoteUsage.

Use common useline notation for available flags:

- `[-f foo]` for (standard) optional flags
- `[-f foo | -b bar]` for mutually exclusive flags
- `-f foo` for required flags
- `{-f foo | -b bar}` for mutually exclusive flags where one is required
- `[-f foo -b bar]` for flags required as a group

For a boolean flag `f`, ` foo` is dropped from the above. The foo/bar
comes from `flag.UnquoteUsage`.
@CLAassistant
Copy link

CLAassistant commented Feb 1, 2024

CLA assistant check
All committers have signed the CLA.

@marckhouzam
Copy link
Collaborator

This sounds like a nice improvement @lugray. Thanks! I’ll try to find some time in the coming weeks.

Copy link
Contributor

@nirs nirs left a comment

Choose a reason for hiding this comment

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

Thanks @lugray! can you share complete help output of complex commands with many arguments using this feature? How long lines are handled?

For example other tools with complex command lines options using this style use multiple lines to make the output nicer.

This example is using manual formatting - hard work but you have complete control on the output:
https://gitlab.com/nbdkit/libnbd/-/blob/master/copy/main.c?ref_type=heads#L69

@@ -1066,6 +1066,72 @@ func TestFlagsInUsage(t *testing.T) {
checkStringContains(t, output, "[flags]")
}

func TestMutuallyExclusiveFlagsInUsage(t *testing.T) {
rootCmd := &Command{Use: "root", Args: NoArgs, Run: func(*Command, []string) {}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of func(*Command, []string) {}} we can use emptyRun

t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, output, "[--foo | --bar]")
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider more condensed format?

command [--foo|--bar]

popular tools like python argparse use the same format suggested, for example:

$ cat args.py 
import argparse
p = argparse.ArgumentParser()
me = p.add_mutually_exclusive_group()
me.add_argument("--foo")
me.add_argument("--bar")
p.parse_args()

$ python args.py -h | head -1
usage: args.py [-h] [--foo FOO | --bar BAR]

t.Errorf("Unexpected error: %v", err)
}

checkStringContains(t, output, "{--foo | --bar}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to chose {} for required choice from mutually exclusive arguments?

python argparse uses () for the same:

$ cat args.py 
import argparse
p = argparse.ArgumentParser()
me = p.add_mutually_exclusive_group(required=True)
me.add_argument("--foo")
me.add_argument("--bar")
p.parse_args()

$ python args.py -h | head -1
usage: args.py [-h] (--foo FOO | --bar BAR)

if varname != "" {
usage += " " + varname
}
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Using bare return is not consistent with other code added.

} else {
flagsLine += " [" + shortUsage(f) + "]"
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

The code to list the short usage strings is repeated twice, making this function even more complicated and hard to follow. We have here many very short variable names (arg, me, or, gr, fl) and complex string formatting. It is pain to follow this code.

We can make it little better by extracting few helpers. I'm not sure about the names, they can be better.

func groupShortUsage(flags map[string]*flag.Flag, included map[*flag.Flag]struct{}) []string {
    gr := []string{}
    for _, fl := range flags {
        included[fl] = struct{}{}
        gr = append(gr, shortUsage(fl))
    }
    return gr
}

func isOneRequired(f *flag.Flag) bool {
    req, found := f.Annotations[BashCompOneRequiredFlag]
    return found && req[0] == "true"    
}

Now we have only 3 very short variables names which is easy to grasp, the rest is the obvious formatting.

		rag := flagsFromAnnotation(c, f, requiredAsGroup)
		me := flagsFromAnnotation(c, f, mutuallyExclusive)
		or := flagsFromAnnotation(c, f, oneRequired)

		if len(rag) > 0 {
			usages := groupShortUsage(rag, included)
			flagsLine += " [" + strings.Join(usages, " ") + "]"
		} else if len(me) > 0 {
			usages := groupShortUsage(me, included)
			if sameFlags(me, or) {
				flagsLine += " {" + strings.Join(usages, " | ") + "}"
			} else {
				flagsLine += " [" + strings.Join(usages, " | ") + "]"
			}
		} else if isOneRequired(f) {
			flagsLine += " " + shortUsage(f)
		} else {
			flagsLine += " [" + shortUsage(f) + "]"
		}

// DisableVerboseFlagsInUseLine will add only [flags] to the usage line of
// a command when printing help or generating docs instead of a more verbose
// form showing the actual available flags
DisableVerboseFlagsInUseLine bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This is consistent with other options, but this change may visually break existing tools using this library. Should we use EnableVerboseFlagsInUsageLine so one can opt in to use the new feature?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an important point. Let's see where the final change lands and we can decide if we need to be less aggressive in pushing this change

nirs added a commit to nirs/cobra that referenced this pull request May 3, 2024
Add `Annotation` suffix to the private annotations to allow nicer code
using the constants.

For example one can use the current annotation names as a temporary
variable instead of unclear shortcut. Instead of this:

    rag := flagsFromAnnotation(c, f, requiredAsGroup)
    me := flagsFromAnnotation(c, f, mutuallyExclusive)
    or := flagsFromAnnotation(c, f, oneRequired)

We can use now:

    requiredAsGrop := flagsFromAnnotation(c, f, requiredAsGroupAnnotation)
    mutuallyExclusive := flagsFromAnnotation(c, f, mutuallyExclusiveAnnotation)
    oneRequired := flagsFromAnnotation(c, f, oneRequiredAnnotation)

Example taken from spf13#2105.
@nirs nirs mentioned this pull request May 3, 2024
nirs added a commit to nirs/cobra that referenced this pull request May 3, 2024
Add `Annotation` suffix to the private annotations to allow nicer code
using the constants.

For example one can use the current annotation names as a temporary
variable instead of unclear shortcut. Instead of this:

    rag := flagsFromAnnotation(c, f, requiredAsGroup)
    me := flagsFromAnnotation(c, f, mutuallyExclusive)
    or := flagsFromAnnotation(c, f, oneRequired)

We can use now:

    requiredAsGrop := flagsFromAnnotation(c, f, requiredAsGroupAnnotation)
    mutuallyExclusive := flagsFromAnnotation(c, f, mutuallyExclusiveAnnotation)
    oneRequired := flagsFromAnnotation(c, f, oneRequiredAnnotation)

Example taken from spf13#2105.
@marckhouzam
Copy link
Collaborator

marckhouzam commented May 18, 2024

Thanks @lugray! can you share complete help output of complex commands with many arguments using this feature? How long lines are handled?

For example other tools with complex command lines options using this style use multiple lines to make the output nicer.

This example is using manual formatting - hard work but you have complete control on the output: https://gitlab.com/nbdkit/libnbd/-/blob/master/copy/main.c?ref_type=heads#L69

I like the idea behind this improvement. Thanks @lugray !

I do prefer the multi-line format @nirs suggests. Currently for example, here is the change to a helm command help:

# current
helm repo add [NAME] [URL] [flags]

# with this PR
helm repo add [NAME] [URL] [--allow-deprecated-repos] [--burst-limit int] [--ca-file string] [--cert-file string] [--debug] [--force-update] [-h] [--insecure-skip-tls-verify] [--key-file string] [--kube-apiserver string] [--kube-as-group stringArray] [--kube-as-user string] [--kube-ca-file string] [--kube-context string] [--kube-insecure-skip-tls-verify] [--kube-tls-server-name string] [--kube-token string] [--kubeconfig string] [-n string] [--no-update] [--pass-credentials] [--password string] [--password-stdin] [--registry-config string] [--repository-cache string] [--repository-config string] [--username string]

As you can see this is problematic for tools with lots of flags.
Could you use multi-line like @nirs suggests?

The above gives me a couple of ideas:

  1. should we list global flags or should we limit ourselves to local flags and maybe add [global flags] at the end?
  2. it may happen that tools only have a few commands with many flags; in such a case it would be nice to allow to control enabling this feature on a per-command basis also. BUT, with the better formatting @nirs suggests, maybe we won't need this. I'm mentioning it so I don't forget.

marckhouzam pushed a commit that referenced this pull request May 18, 2024
Add `Annotation` suffix to the private annotations to allow nicer code
using the constants.

For example one can use the current annotation names as a temporary
variable instead of unclear shortcut. Instead of this:

    rag := flagsFromAnnotation(c, f, requiredAsGroup)
    me := flagsFromAnnotation(c, f, mutuallyExclusive)
    or := flagsFromAnnotation(c, f, oneRequired)

We can use now:

    requiredAsGrop := flagsFromAnnotation(c, f, requiredAsGroupAnnotation)
    mutuallyExclusive := flagsFromAnnotation(c, f, mutuallyExclusiveAnnotation)
    oneRequired := flagsFromAnnotation(c, f, oneRequiredAnnotation)

Example taken from #2105.
@nirs
Copy link
Contributor

nirs commented May 18, 2024

Trying helm help with multiple lines (manually formatted):

$ helm repo add -h
add a chart repository

Usage:
  helm repo add [NAME] [URL] [--allow-deprecated-repos] [--burst-limit int] [--ca-file string]
                [--cert-file string] [--debug] [--force-update] [-h]
                [--insecure-skip-tls-verify] [--key-file string] [--kube-apiserver string]
                [--kube-as-group stringArray] [--kube-as-user string] [--kube-ca-file string]
                [--kube-context string] [--kube-insecure-skip-tls-verify]
                [--kube-tls-server-name string] [--kube-token string] [--kubeconfig string]
                [-n string] [--no-update] [--pass-credentials] [--password string]
                [--password-stdin] [--registry-config string] [--repository-cache string]
                [--repository-config string] [--username string]

Flags:
      --allow-deprecated-repos     by default, this command will not allow adding official repos that have been permanently deleted. This disables that behavior
      --ca-file string             verify certificates of HTTPS-enabled servers using this CA bundle
      --cert-file string           identify HTTPS client using this SSL certificate file
      --force-update               replace (overwrite) the repo if it already exists
  -h, --help                       help for add
      --insecure-skip-tls-verify   skip tls certificate checks for the repository
      --key-file string            identify HTTPS client using this SSL key file
      --no-update                  Ignored. Formerly, it would disabled forced updates. It is deprecated by force-update.
      --pass-credentials           pass credentials to all domains
      --password string            chart repository password
      --password-stdin             read chart repository password from stdin
      --username string            chart repository username

Global Flags:
      --burst-limit int                 client-side default throttling limit (default 100)
      --debug                           enable verbose output
      --kube-apiserver string           the address and the port for the Kubernetes API server
      --kube-as-group stringArray       group to impersonate for the operation, this flag can be repeated to specify multiple groups.
      --kube-as-user string             username to impersonate for the operation
      --kube-ca-file string             the certificate authority file for the Kubernetes API server connection
      --kube-context string             name of the kubeconfig context to use
      --kube-insecure-skip-tls-verify   if true, the Kubernetes API server's certificate will not be checked for validity. This will make your HTTPS connections insecure
      --kube-tls-server-name string     server name to use for Kubernetes API server certificate validation. If it is not provided, the hostname used to contact the server is used
      --kube-token string               bearer token used for authentication
      --kubeconfig string               path to the kubeconfig file
  -n, --namespace string                namespace scope for this request
      --registry-config string          path to the registry config file (default "/home/nsoffer/.config/helm/registry/config.json")
      --repository-cache string         path to the file containing cached repository indexes (default "/home/nsoffer/.cache/helm/repository")
      --repository-config string        path to the file containing repository names and URLs (default "/home/nsoffer/.config/helm/repositories.yaml")

Much better than long line but still overwhelming. The current output is better. Finding the right flag in the list of flags is much easier.

$ helm repo add -h
add a chart repository

Usage:
  helm repo add [NAME] [URL] [flags]

Flags:
      --allow-deprecated-repos     by default, this command will not allow adding official repos that have been permanently deleted. This disables that behavior
      --ca-file string             verify certificates of HTTPS-enabled servers using this CA bundle
      --cert-file string           identify HTTPS client using this SSL certificate file
      --force-update               replace (overwrite) the repo if it already exists
  -h, --help                       help for add
      --insecure-skip-tls-verify   skip tls certificate checks for the repository
      --key-file string            identify HTTPS client using this SSL key file
      --no-update                  Ignored. Formerly, it would disabled forced updates. It is deprecated by force-update.
      --pass-credentials           pass credentials to all domains
      --password string            chart repository password
      --password-stdin             read chart repository password from stdin
      --username string            chart repository username

Global Flags:
      --burst-limit int                 client-side default throttling limit (default 100)
      --debug                           enable verbose output
      --kube-apiserver string           the address and the port for the Kubernetes API server
      --kube-as-group stringArray       group to impersonate for the operation, this flag can be repeated to specify multiple groups.
      --kube-as-user string             username to impersonate for the operation
      --kube-ca-file string             the certificate authority file for the Kubernetes API server connection
      --kube-context string             name of the kubeconfig context to use
      --kube-insecure-skip-tls-verify   if true, the Kubernetes API server's certificate will not be checked for validity. This will make your HTTPS connections insecure
      --kube-tls-server-name string     server name to use for Kubernetes API server certificate validation. If it is not provided, the hostname used to contact the server is used
      --kube-token string               bearer token used for authentication
      --kubeconfig string               path to the kubeconfig file
  -n, --namespace string                namespace scope for this request
      --registry-config string          path to the registry config file (default "/home/nsoffer/.config/helm/registry/config.json")
      --repository-cache string         path to the file containing cached repository indexes (default "/home/nsoffer/.cache/helm/repository")
      --repository-config string        path to the file containing repository names and URLs (default "/home/nsoffer/.config/helm/repositories.yaml")

Trying without global flags looks much better:

$ helm repo add -h
add a chart repository

Usage:
  helm repo add [NAME] [URL] [--allow-deprecated-repos] [--ca-file string]
                [--cert-file string] [--force-update] [-h] [--insecure-skip-tls-verify]
                [--key-file string] [--no-update] [--pass-credentials] [--password string]
                [--password-stdin] [--username string]

Flags:
      --allow-deprecated-repos     by default, this command will not allow adding official repos that have been permanently deleted. This disables that behavior
      --ca-file string             verify certificates of HTTPS-enabled servers using this CA bundle
      --cert-file string           identify HTTPS client using this SSL certificate file
      --force-update               replace (overwrite) the repo if it already exists
  -h, --help                       help for add
      --insecure-skip-tls-verify   skip tls certificate checks for the repository
      --key-file string            identify HTTPS client using this SSL key file
      --no-update                  Ignored. Formerly, it would disabled forced updates. It is deprecated by force-update.
      --pass-credentials           pass credentials to all domains
      --password string            chart repository password
      --password-stdin             read chart repository password from stdin
      --username string            chart repository username

Global Flags:
      --burst-limit int                 client-side default throttling limit (default 100)
      --debug                           enable verbose output
      --kube-apiserver string           the address and the port for the Kubernetes API server
      --kube-as-group stringArray       group to impersonate for the operation, this flag can be repeated to specify multiple groups.
      --kube-as-user string             username to impersonate for the operation
      --kube-ca-file string             the certificate authority file for the Kubernetes API server connection
      --kube-context string             name of the kubeconfig context to use
      --kube-insecure-skip-tls-verify   if true, the Kubernetes API server's certificate will not be checked for validity. This will make your HTTPS connections insecure
      --kube-tls-server-name string     server name to use for Kubernetes API server certificate validation. If it is not provided, the hostname used to contact the server is used
      --kube-token string               bearer token used for authentication
      --kubeconfig string               path to the kubeconfig file
  -n, --namespace string                namespace scope for this request
      --registry-config string          path to the registry config file (default "/home/nsoffer/.config/helm/registry/config.json")
      --repository-cache string         path to the file containing cached repository indexes (default "/home/nsoffer/.cache/helm/repository")
      --repository-config string        path to the file containing repository names and URLs (default "/home/nsoffer/.config/helm/repositories.yaml")

Maybe we need 2 options?

  • ShowFlagsInUseLine
  • ShowPersistentFlagsInUseLine

Another way , add MarkShowFlagInUseLine() so it is possible to include or exclude flags from the use line to show only the most important flags in the use line. This allows making insecure options like [--insecure-skip-tls-verify] less visible, helping users to do the right thing.

@marckhouzam
Copy link
Collaborator

Seeing the whole output makes me realize we have a duplication of flag names very close together (in the usage line and listed below it). This makes me wonder if the proposed change brings much value to the end user?

I’m leaning towards keeping things as is to avoid unnecessarily complicating the code base.

But I’m open to hear if someone sees more value in this modified help text.

@marckhouzam
Copy link
Collaborator

I'm going to close this as "won't implement".
If you still feel this has value, feel free to re-open and explain.

@marckhouzam marckhouzam closed this Jul 8, 2024
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.

4 participants