cobra icon indicating copy to clipboard operation
cobra copied to clipboard

Inconsistent returned data from flags StringP

Open optik-aper opened this issue 2 years ago • 6 comments

I'm noticing that when I pass nothing (or an emptry string) to the shortened version of a flag I'm not getting nothing from Flags.GetString. When I use the full flag, I do get an empty string.

With this command:

var reservedIPUpdate = &cobra.Command{
	Use:   "update <reservedIPID>",
	Short: "update reservedIP",
	Long:  ``,
	Args: func(cmd *cobra.Command, args []string) error {
		if len(args) < 1 {
			return errors.New("please provide a reserved IP ID")
		}
		return nil
	},
	Run: func(cmd *cobra.Command, args []string) {
		label, _ := cmd.Flags().GetString("label")
		ip := args[0]

		fmt.Printf("label: %v", label)
		fmt.Println()

		options := &govultr.ReservedIPUpdateReq{
			Label: govultr.StringToStringPtr(label),
		}

		r, err := client.ReservedIP.Update(context.Background(), ip, options)
		if err != nil {
			fmt.Printf("error updating reserved IPs : %v\n", err)
			os.Exit(1)
		}

		printer.ReservedIP(r)
	},
}

Defined with one flag:

	// Update
	reservedIPUpdate.Flags().StringP("label", "l", "", "label")
	reservedIPUpdate.MarkFlagRequired("label")

When I run it like: go run main.go reserved-ip <UUID> -l="" prints label: = But when I run it with the full name flag: go run main.go reserved-ip <UUID> --label="" prints label:

What's the difference between the shortened version and the full flag? I would expect these to return the same value in this case.

optik-aper avatar Jun 10 '22 20:06 optik-aper

Thanks for the issue.

I googled a bit and found an 'open CLI' page (not necessarily the standard though, I'm unfamiliar with them) but it says:

A flag and its option can be separated by a space or an equals sign =. Interestingly, short flags (but not long flags) can even skip the space, although many people find it much easier to read with the space or equals sign.

These three are all valid and equivalent:

tar -f archive.tar
tar -f=archive.tar
tar -farchive.tar

Which would line up with what you're reporting.

The issue is only with an edge case; could you have a short form string value that actually starts with the =? E.g. (-i=foo and I want the value to actually be =foo and not foo). If that were the case you could still do -i==foo so no one is blocked.

I tend to agree with you that this is a bug unless someone can point to documentation that says otherwise.

johnSchnake avatar Jun 13 '22 20:06 johnSchnake

This is probably just an issue in/with spf13/pflag but its not uncommon for the issues to be reported here. Just FYI we may move it to that repo.

johnSchnake avatar Jun 13 '22 20:06 johnSchnake

https://github.com/spf13/pflag#command-line-flag-syntax even shows the -f=foo' and '-f foo options, so it does seem to be a bug, not a difference in intent or design.

johnSchnake avatar Jun 13 '22 20:06 johnSchnake

Thanks for looking into it and confirming the behavior. It's not a critical issue for me--just something I noticed--so feel free to move/close/etc. however makes sense.

optik-aper avatar Jun 13 '22 20:06 optik-aper

I dont have much time to actually debug or code it right now but a breadcrumb for if anyone wants to fix it, the logic is here for the short flag parsing: https://github.com/spf13/pflag/blob/master/flag.go#L1047-L1067 then they'd need to ensure the test cases are covered in flag_test.go (I'm guessing there are similar tests already)

johnSchnake avatar Jun 13 '22 21:06 johnSchnake

The pflag repo links this GNU manual for option parsing: https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html

An option and its argument may or may not appear as separate tokens. (In other words, the whitespace separating them is optional.) Thus, -o foo and -ofoo are equivalent.

In fact the manual doesn't even say -o=foo valid, the equal sign is always treated as part of the value, .e.g. cut(1):

$ cut -d: -f=1 /etc/passwd
cut: invalid field value ‘=1’
$ cut -d: -f1 /etc/passwd
root
paul

So the fact that -o=foo gives us foo as flag value is wrong per GNU options.

Btw, -l="" is the same as -l= since your shell will remove the quotes, however -l"" and -l "" are different since the second one creates an empty string as argument.

Luap99 avatar Jun 18 '22 09:06 Luap99

The Cobra project currently lacks enough contributors to adequately respond to all issues. 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 issue is closed. You can:
  • Make a comment to remove the stale label and show your support. The 60 days reset. - If an issue has lifecycle/rotten and is closed, comment and ask maintainers if they'd be interseted in reopening

github-actions[bot] avatar Aug 18 '22 00:08 github-actions[bot]