integrii/flaggy

Displays current value as "Default" on Subcommand Positional Value when Extra

donatj opened this issue · 6 comments

donatj commented

When using positional values, if I accidentally add an extra argument the "default" is shown as whatever I entered already for the existing positional arguments.

You can see from the following example program the "id" field shows the executed "bad" value as the default.

package main

import "github.com/integrii/flaggy"

func main() {
	var id string

	getCmd := flaggy.NewSubcommand("get")
	getCmd.Description = "Get from id"
	getCmd.AddPositionalValue(&id, "id", 1, true, "ID")
	flaggy.AttachSubcommand(getCmd, 1)

	flaggy.Parse()
}
$ ./main get bad extra
get - Get from id

  Usage:
    get [id]

  Positional Variables:
    id (Required) - ID (default: bad)
  Flags:
       --version  Displays the program version string.
    -h --help  Displays help with available flag, subcommand, and positional value parameters.

Unexpected argument: extra
integrii commented

Thanks! I will look into getting this patched up. I appreciate the clear report with example code, too.

integrii commented

I am going to think on this one a little more before I make any changes... The code works like this:

  • When its time to show help:
  • loop over every flag and positional
  • look at the current value of the assignment variable
  • set that value as the default value
  • display help

This obviously does not work when flaggy exits in the middle of parsing flags... Some of the assignment vars may have been set and the default values are long gone.

I considered snapshotting all variables before parsing any subcommand, but subcommands can be nested, so it is possible that the subcommand snapshots a variable that has already been assigned from a parent subcommand.

It seems like the best way to solve this is to write a recursive crawler that crawls down all possible paths including their flags and positionals in order to build a snapshot of all the default values. This way, when help does fire, we can go back and look up what things were before anything was parsed. This could be a pretty serious feature though, and will likely impact performance and code complexity.

Thinkin...

donatj commented

lol - honestly it's no skin off my back, I just noticed it when I typo'd a command but it doesn't really hurt me any.

I wouldn't want it to needlessly overcomplicate the code 😄

integrii commented

Yeah, I don't want to either. I need to solve it simply. I can't leave the feature broken, though. I will eventually get to this some weekend when I have downtime and thinking comes easy.

integrii commented

This took some careful thought, but simply saving the original state the first time a variable is parsed, then using that in help output solves the problem. No crazy recursion required. This is now in the unstable branch for longer periods of testing.

donatj commented