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

How do I define Usage for BoolWithInverseFlag? #1883

Open
Skeeve opened this issue Apr 4, 2024 · 23 comments
Open

How do I define Usage for BoolWithInverseFlag? #1883

Skeeve opened this issue Apr 4, 2024 · 23 comments
Labels
area/v3 relates to / is being considered for v3 good first issue Easy fix for newcomers kind/bug describes or fixes a bug

Comments

@Skeeve
Copy link

Skeeve commented Apr 4, 2024

I defined a "summary" boolean flag as a BoolWithInverseFlag.

But the help/usage text doesn't look very helpful to me:

package main

import (
	"context"

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

func main() {
	cmd := &cli.Command{
		Name:                   "sendsomething",
		Usage:                  "report some stuff",
		ArgsUsage:              "",
		HideHelpCommand:        true,
		UseShortOptionHandling: true,
		Flags: []cli.Flag{
			&cli.BoolFlag{
				Name:    "verbose",
				Aliases: []string{"v"},
				Usage:   "Show more output.",
			},
			&cli.BoolFlag{
				Name:    "dry-run",
				Aliases: []string{"dryrun", "d"},
				Usage:   "Do a dry-run.",
			},
			&cli.StringSliceFlag{
				Name:  "to",
				Usage: "Define alternative recipients.",
			},
			&cli.BoolWithInverseFlag{
				BoolFlag: &cli.BoolFlag{
					Name:  "summary",
					Usage: "default is, to send a summary to all subscribers.",
				},
			},
		},
	}
	_ = cmd.Run(context.Background(), []string{"prog", "--help"})
}

See on Go Playground

Actual output

NAME:
   sendsomething - report some stuff

USAGE:
   sendsomething [global options] [arguments...]

GLOBAL OPTIONS:
   --verbose, -v              Show more output. (default: false)
   --dry-run, --dryrun, -d    Do a dry-run. (default: false)
   --to value [ --to value ]  Define alternative recipients.
   --summary                  default is, to send a summary to all subscribers. (default: false) || --no-summary  default is, to send a summary to all subscribers. (default: false)
   --help, -h                 show help (default: false)

Expected output

This is the output I expected:

NAME:
   sendsomething - report some stuff

USAGE:
   sendsomething [global options] [arguments...]

GLOBAL OPTIONS:
   --verbose, -v              Show more output. (default: false)
   --dry-run, --dryrun, -d    Do a dry-run. (default: false)
   --to value [ --to value ]  Define alternative recipients.
   --summary                  Only send the summary. (default: false)
   --no-summary               Suppress the default summary. (default: false)
   --help, -h                 show help (default: false)

So far, with my current knowledge, I can only achieve it by defining two separate BoolFlags, which is an ugly workaround.

Am I misundertstanding the BoolWithInverseFlag? Am I using it wrong?

@Skeeve Skeeve added area/v2 relates to / is being considered for v2 kind/question someone asking a question status/triage maintainers still need to look into this labels Apr 4, 2024
@dearchap
Copy link
Contributor

@Skeeve yes that is the current behavior, you are using the BoolWithInverseFlag correctly. I think what you want is the output to have "--no-summary" on a different line correct ? You might be able to modify the help template to get what you want. Let me take a look and see what can be done.

@Skeeve
Copy link
Author

Skeeve commented Apr 16, 2024

Yes. On a different line and with a different text.

@bartekpacia bartekpacia added area/v3 relates to / is being considered for v3 kind/bug describes or fixes a bug and removed area/v2 relates to / is being considered for v2 status/triage maintainers still need to look into this kind/question someone asking a question labels May 2, 2024
@bartekpacia
Copy link
Contributor

I am changing labels on this issue to target v3, because that's what the code in the issue uses. I think this issue was classified as v2 by mistake.

I also reworded the issue a bit.

@bartekpacia
Copy link
Contributor

bartekpacia commented May 2, 2024

@Skeeve, would this be OK for you? This is how Dart's package:args does this, and I quite like it:

NAME:
   sendsomething - report some stuff

USAGE:
   sendsomething [global options] [arguments...]

GLOBAL OPTIONS:
   --verbose, -v              Show more output. (default: false)
   --dry-run, --dryrun, -d    Do a dry-run. (default: false)
   --to value [ --to value ]  Define alternative recipients.
   --[no-]summary             Only send the summary. (default: false)
   --help, -h                 show help (default: false)

The problem with your example in the issue is: where to define the second Usage string? You mention:

  • "Only send the summary." for --summary
  • "Suppress the default summary." for --no-summary

but currently BoolWithInverseFlag only accepts a single Usage.

But your use case seems that semantically, you have 3 actions for "summary": "suppress the default summary", something in the middle, and "only send the summary". If this is the case then I'm afraid BoolWithInverseFlag won't help you.

@bartekpacia bartekpacia added the good first issue Easy fix for newcomers label May 2, 2024
@Skeeve
Copy link
Author

Skeeve commented May 2, 2024

You are right with your assumption.

  • --summary: send only the summary, i.e. suppress the other mails
  • --no-summary: send no summary, i.e. send only the other mails
  • none of them: Send everything

So I think I have to stick with defining two flags.

But to answer your question: I think, that option would be better than what cli has now.

@bartekpacia
Copy link
Contributor

Would you be up to submit a Pull Request with a fix?

@Skeeve
Copy link
Author

Skeeve commented May 3, 2024

Would you be up to submit a Pull Request with a fix?

I didn't change anything in cli, so no. I cannot submit a pull request. It's just that I define two normal bool flags in my program.

@bartekpacia
Copy link
Contributor

Yeah yeah – I meant "are you willing to modify urfave/cli code that fixes how help is displayed?".

@Skeeve
Copy link
Author

Skeeve commented May 3, 2024

Sorry, I don't get it. I'm not a native speaker, so I think I need more explanation what you want me to do.

@bartekpacia
Copy link
Contributor

No problem :) That's okay.

Would you like to make an open-source contribution? More specifically:

  1. You fork urfave/cli
  2. You fix the bug in urfave/cli you described (so BoolWithInverseFlag will display correctly as --[no-]summary in --help)
  3. You submit a Pull Request in this GitHub repository with the bugfix.

This is what I mean. I hope it's easier to understand now.

Ah, and I don't want you to do anything. I'm only asking.

@Skeeve
Copy link
Author

Skeeve commented May 3, 2024

Okay. Understood. Will think about it. I will try to find some time on Sunday to look into that. Will be a good training in understanding GO for me.

@Skeeve
Copy link
Author

Skeeve commented May 3, 2024

@bartekpacia I thought a bit about it and in fact, I wouldn't need to define two variables as the BoolWithInversFlag already has three states:

https://play.golang.com/p/MetA6KyQmM_f

  • not set
  • set to true
  • set to false

which is exactly what I want. Remains the issue: how to define different texts. But I think this is just a case of proper wording.

Additionally the (default: false) shouldn't be shown in the help unless a default was explicitly set.

If that's okay with you, I'll try my luck ;) at coding it.

@bartekpacia
Copy link
Contributor

bartekpacia commented May 3, 2024

That's an interesting find. If it solves your problem, that's great.

But I'm not sure if it's "the way" to solve this problem – I think it's no longer "declarative" then.

Maybe a new BoolWithInverseFlag (or OptionalBool?) function should be created to allow for retrieval of the information you need:

Example
package main

import (
	"context"
	"fmt"

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

func main() {
	cmd := &cli.Command{
		Flags: []cli.Flag{
			&cli.BoolWithInverseFlag{
				BoolFlag: &cli.BoolFlag{
					Name: "env",
				},
			},
		},
		Action: func(_ context.Context, cmd *cli.Command) error {
			var value *bool
			value = cmd.BoolWithInverse("env")

			if value != nil {
				if value {
					fmt.Println("env is set to true")
				} else {
					fmt.Println("env is set to false")
				}
			} else {
				fmt.Println("env is not set")
			}

			return nil
		},
	}

	_ = cmd.Run(context.Background(), []string{"prog"})
	_ = cmd.Run(context.Background(), []string{"prog", "--no-env"})
	_ = cmd.Run(context.Background(), []string{"prog", "--env"})

	fmt.Println("flags:", len(flagWithInverse.Flags()))
}

If that's okay with you, I'll try my luck ;) at coding it.

Sure! This is an open-source project, the more people submit fixes (even small ones), the better for everyone.

@Skeeve
Copy link
Author

Skeeve commented May 3, 2024

I think it's no longer "declarative" then.

Why? What bothers you? That the default should only be shown if a default is set? To me this seems quite logical.

Or is there anything else which doesn't feel right for you?

@Skeeve
Copy link
Author

Skeeve commented May 5, 2024

Maybe a new BoolWithInverseFlag (or OptionalBool?) function should be created to allow for retrieval of the information you need

The current BoolFlag already has all the information I need.

  1. You provide the flag -> The value will be true and iISet() will be true
  2. You do not provide the flag -> The value will be false and IsSet() will be false

My understanding of the BoolWithInverseFlag is, that it only provides a third option:

  1. You provide the negative flag -> The value will be false and IsSet() will be true

If the bool is a counter, the value will be a positive or negative number depending on the amount of times it was given. Maybe even 0 if positive and negative were given the same number of times.

I had a quick look at BoolWithInverseFlag but couldn't understand the implementation fully. To me it seems as if it simply combines two BoolFlags which is, I think, overvomplicated. I think a simple Bool would do. The only difference is, that the cli parser needs to know that the negative switch needs to be taken into account.

@Skeeve
Copy link
Author

Skeeve commented May 5, 2024

P.S. Can you please educate me on how to run tests? Maybe the question sounds dumb, but I'm still new to go.

@bartekpacia
Copy link
Contributor

bartekpacia commented May 5, 2024

@Skeeve, thank you very much for looking into the code and sharing your thoughts. Indeed I think something may be wrong – maybe we don't need BoolWithInverseFlag at all? v3 is still in alpha, so we can still change it.

Can you please educate me on how to run tests? Maybe the question sounds dumb, but I'm still new to go.

Sure! To run all tests in all packages in the directory, do:

go test ./...

But in case of urfave/cli, which is only a single package, you can do this:

go test . 

I hope it helps :) See also this StackOverflow answer. If you have any more questions just ask!

@Skeeve
Copy link
Author

Skeeve commented May 5, 2024

maybe we don't need BoolWithInverseFlag at all? v3 is still in alpha, so we can still change it.

I do think so, provided it's okay if every BoolFlag has a negative counterpart.

On the other hand:

Maybe the config options WithInverse bool and an optional InverseUsage string for BoolFag could solve what I found.

If WithInverse is true, the flag has an inverse. Otherwise it will be the same as the current BoolFlag.

If InverseUsage is defined, it gives the usage text for the --no- flag.

We need to also allow the simultaneous use of --no-env and --env. I think it makes sense given that default values for flags can be set via config file or environment. So having --no-env as a default config defined, it would be possible to use --env on the commandline.

@bartekpacia
Copy link
Contributor

Yes, I agree with almost everything that you wrote above. I'm curious what other maintainers think.

One thing I don't agree with is this:

We need to also allow the simultaneous use of --no-env and --env.

At first glance, it seems illogical to me.

@Skeeve
Copy link
Author

Skeeve commented May 5, 2024

At first glance, it seems illogical to me.

Didn‘t my argument make any sense? About having an environment variable or a default config file defining (for example) —env while you want to have —no-env in the call?

@bartekpacia
Copy link
Contributor

In general, arguments/flags passed as arguments on the command-line are given higher priority than the ones in the config file. So in your example, I'd expect --env from the config file to be ignored, and only --no-env to be taken into account.

Frankly, I've never used altsrc with urfave/cli, so I'm not experienced with it. Maybe you could show an example of what you mean?

@Skeeve
Copy link
Author

Skeeve commented May 5, 2024

Frankly, I've never used altsrc with urfave/cli, so I'm not experienced with it. Maybe you could show an example of what you mean?

Me neither. But if what you say is true, then I think my arguments don’t count.

@Skeeve
Copy link
Author

Skeeve commented May 6, 2024

I'm curious what other maintainers think.

So how do we find out?

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 good first issue Easy fix for newcomers kind/bug describes or fixes a bug
Projects
None yet
Development

No branches or pull requests

3 participants