cli icon indicating copy to clipboard operation
cli copied to clipboard

Args (an alternative to Flags)

Open Nokel81 opened this issue 4 years ago • 22 comments

Checklist

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

What problem does this solve?

Undocumented arguments can be a burden and forces developers to generate write their own ArgsUsage

Solution description

Add a new field to App and Command called Args of type []ArgSpec which is an interface like Flag. There would many structs that implement this interface, like those for parsing flags.

type ArgSpec interface {
    // Name returns the name that this arg will be accessed by
    Name() string
    // Required returns true if this arg is required 
    Required() bool
    // Priority means the order (highest to lowest) which are parsed after the required ones
    Priority() uint
    // Min returns the minimum args to be parsed
    Min() uint
    // Max returns the maximum args to be parsed, if 0 then any number can be parsed 
    Max() uint
}

Along with this there are several other rules that will result in a panic (always no matter what the provided args to be parsed are)

  • All Optional fields must either come before all required fields or after all required fields
  • At most one Max() == 0 arg per field

Args never mean anything for sub-comands.

Args will be accessed in the same manner as Flags currently, they are parsed after flags so will override them (that is how one can have a EnvVar be the default to an arg)

Implementation

If there are any []ArgSpec then all provided args must be used, otherwise a error is printed as if a required flag was not supplied.

This also means that the Arg(n) and Args() won't return anything after parsing args.

Describe alternatives you've considered

  • Do nothing, this a hard problem to solve and this solution is complicated to use.

Nokel81 avatar Feb 24 '20 22:02 Nokel81

I think this idea will need a few iterations - but it's off to a good start! I think we should really consider something like this 👍

coilysiren avatar Feb 25 '20 07:02 coilysiren

Hey, I think this is a good idea, a few thoughts from my side:

  • I think we should call them positional arguments
  • Positional arguments can only apply to commands
  • They should support specific value sets, like we do for flags
  • I think we do not need an index if we inherit that information from a slice index (if app.PositionalArgs = []cli.PositionalArg{}

saschagrunert avatar Feb 25 '20 07:02 saschagrunert

  1. I like the name positional arguments
  2. Would that mean that an application like ls would not be able to be written using this feature?
  3. And specific types?
  4. I agree we would not need indexes anymore, I was thinking of having them accessible by name using the *Command.<Type>() methods.

Having done some more preliminary investigation by doing a mock implementation I have found the following to be necessary:

  • Remove "priority" and have it strictly left to right
  • Only the last "required" pos-arg can be of slice + unlim length
  • There is no parsing of slice args based on commas, just on the args specified whitespace (namely 1.0 2.0 3.0 would be the three elements of a float64slice
  • All previous positional args get filled before going on to the next (important for slices)

Nokel81 avatar Feb 25 '20 12:02 Nokel81

I'm marking this as help wanted for someone to work on the design, since it'll be a few steps before this is ready for implementation

coilysiren avatar Feb 26 '20 05:02 coilysiren

Nice proposal! Just some drive-by-bikeshedding from a random user:

Wouldn't naming the interface Args instead of ArgSpec fit better with the existing Flag interface? Maybe even Arguments or ArgumentGroup, since this wouldn't be typed often enough to justify sacrificing readability for "writeability".

Also, wouldn't it be possible to implement Required: true via Min: 1? This would leave us with a leaner interface:

type ArgumentGroup interface {
  Name() string
  Min() uint
  Max() uint
}

And as a bonus we wouldn't have to deal with the corner-case of Required: true + Min: 0.

Implementing something like mv or cp would be a matter of:

app := &cli.App{
	Name:    "cp",
	Arguments: []cli.ArgumentGroup{
		cli.StringArguments{
			Name: "paths",
			Min: 2,
		},
	},
}

(assuming cli.StringArgument could default to Max:0)

But I wonder a bit what the main use-case for multiple argument groups would be. The relatively common cmd [foo [bar]] idiom could be implemented using multiple groups with something like:

app := &cli.App{
	Name:    "cmd",
	Arguments: []cli.ArgumentGroup{
		cli.OptionalStringArgument{
			Name: "foo",
		},
		cli.OptionalStringArgument{
			Name: "bar",
		},
	},
}

Where cli.OptionalArgument is a cli.ArgumentGroup with Min() → 0 and Max() → 1 (assuming such a helper would be acceptable).

But OTOH, the same could be accomplished with a single argument group and simple slice logic. I'm not sure the increase in complexity is worth it, just to get a more comfortable access pattern for the individual arguments. :thinking:

costela avatar Feb 26 '20 16:02 costela

We could also allow for some sort of regular expression based system where the Argument has a "is valid" method

Nokel81 avatar Mar 05 '20 19:03 Nokel81

Whatever we do for this feature request (eg. https://github.com/urfave/cli/issues/1074), it should also most likely solve this request too => https://github.com/urfave/cli/issues/1069

coilysiren avatar Mar 30 '20 23:03 coilysiren

Current idea formulation:

Arguments have at least the following:

  • func PlacementName() rune: this value is the value that must be used in the argument placement regex. It is a singular rune because that makes the regex both faster and not ambiguous. If any of these rune's collide then a panic will happen at run-time.
  • func IsValid(arg string) bool: this function is used to build up the placement string for the regex matching. This must be required to be stateless and deterministic. It should also be cheap to run, since it will be run multiple times.
  • func DocName() string: this function is used in the generation of documentation.

Using this idea:

Arguments will be provided in some manner. The ordering doesn't matter at all as all arguments will be "placed" (like flags but I think it should be required).

With the arguments, a "placement string" must also be supplied. The syntax is a subset of full regular expressions.

  • All the counting formulations are allowed: one, ?, *, +, {n, m}, etc....
  • Only "single character" character classes or | not [...] nor [^...].
  • No escape sequences or ^ or $ (those are assumed always)
  • No regex flags
  • No whitespace
  • The only characters allowed are those that are returned by the PlacementName() methods on the arguments for a specific command.

The following is then done in order:

  1. The set of "possible placement" strings are constructed based on the IsValid() method and the "placement string". Creating a string that looks like a concatenation of possible values of that argument, for example if arg[1] could either be A or B the resulting substring would be [AB]. If arg[2] could either be A or C or H then the resulting substring would be [ACH].
  2. The placement string is transformed into matches that match those substrings for each type. So for the placement of A+ it would be transformed into ((?:\[[^\]]*(?:A)[^\]*]*\])+). This way each substring can be match for a specific type. This is also why the placement name must be single characters, that way this regex is unambiguous. This is called the "matching string".
    • Those characters are implementation details, the documentation generation should use the argument's display name.
    • Example 2: (A|B)+: ((?:\[[^\]]*(?:A|B)[^\]*]*\])+). If there is some ambiguity (for example an argument gets assigned the pps of [AB] then the precedence is left->right.
  3. Since each argument's "type" is represented by a string of the form A(|B)* (where A and B are any character (and the repeats are different) once a match has been found the ordering from left to right is used to choose which type the match really refers too.

Example:

  1. placement string: AB{1, 3}A
  2. args:
  • A: Int32
  • B: string
  1. input: 31 h 2 "hello world" 789
  2. Possible placement strings: [AB][B][AB][B][AB]
  3. Matching string: ^((?:\[[^\]]*(?:A)[^\]*]*\]))((?:\[[^\]]*(?:B)[^\]*]*\]){1,3})((?:\[[^\]]*(?:A)[^\]*]*\]))$
  4. Which gives the following substring matches:
    1. [AB][A][BC][B][AB]
    2. [AB]
    3. [B][AB][B]
    4. [AB]
  5. Then each substring match (the part enclosed by [...]) are checked to decide which type to use. In this example, we find it to be: A, B, B, B, A.
  6. The argument parsing and placing is done

Nokel81 avatar Mar 31 '20 18:03 Nokel81

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

stale[bot] avatar Jun 30 '20 15:06 stale[bot]

I'm still super interested in seeing this happen ^^

coilysiren avatar Jul 02 '20 03:07 coilysiren

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

stale[bot] avatar Jul 02 '20 03:07 stale[bot]

I would really like to see this too

clarktlaugh avatar Aug 04 '20 01:08 clarktlaugh

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

stale[bot] avatar Nov 02 '20 03:11 stale[bot]

Closing this as it has become stale.

stale[bot] avatar Dec 03 '20 05:12 stale[bot]

👀

coilysiren avatar Jan 15 '21 20:01 coilysiren

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

stale[bot] avatar Jan 15 '21 20:01 stale[bot]

As far as I remember, we would love to see this but it needs more design work 🙏🏼

coilysiren avatar Jan 15 '21 20:01 coilysiren

This is a good feature to have. Theoretically this feature could be implemented in the Before function of the command with a standard set of validators, regex and filepath validator, so if the command is expecting arguments like

[param1] [param2] [file1] ....

It should work just fine. This along with a ExpectedMinNumArgs field in the command would solve the majority of the use cases for most people.

dearchap avatar Feb 08 '21 01:02 dearchap

Here's my design take on full end to end flow of new Args feature

  // Arg specifies one group of arguments
  type Arg interface {
	  Parse(a cli.Args) (cli.Args, error)
	  Name() string
	  Usage() string
  }
  
// IntArg presents an int argument
// Similar for other types
type IntArg struct {
	Name  string
	Value int64
	Destination *int64
}

type ArgsSet interface {
	// Get returns the nth argument, or else a blank string
	Get(n int) Arg
	// First returns the first argument, or else a blank string
	First() Arg
	// Tail returns the rest of the arguments (not the first one)
	// or else an empty string slice
	Tail() []Arg
	// Len returns the length of the wrapped slice
	Len() int
	// Present checks if there are any arguments present
	Present() bool
	// Slice returns a copy of the internal slice
	Slice() []Arg
	// Returns arg by name
	ByName(s string) Arg
}

// this will implement ArgSet interface
type argSet []*Arg

func (s argSet) parse(a cli.Args) (cli.Args, error) {
	for _, arg := range s {
		var err error
		if a, err = arg.Parse(a); err != nil {
			return a, err
		}
	}
	return a, nil
}


func setupCommand() *cli.Command {
	return &cli.Command {
		ArgSet: []*Arg {
			&IntArg{
				Name: "foo",
				// and other fields
			}
		},
		Action: func(ctx *cli.Context) error {
			int val = ctx.Command.First().(IntArg).Value
			// do something with value
		}
	}
}

func (c *Command) Run(ctx *Context) (err error) {

	args, err := c.ArgSet.parse(ctx.Args())
	if err != nil {
		_ = ShowCommandHelp(context, c.Name)
		return err
	}
	context.Command = c
	err = c.Action(context)

}

dearchap avatar Mar 21 '21 12:03 dearchap

Random FYI, people have since created https://github.com/urfave/cli/issues/1237 and https://github.com/urfave/cli/issues/1246 which as describing essentially the same functionality that's described here

coilysiren avatar Apr 27 '21 22:04 coilysiren

My understanding is that we would still love to see this happen, we just need someone to volunteer to take on the work of designing and building that feature 🙏🏼

coilysiren avatar Apr 27 '21 22:04 coilysiren

As I mentioned in #1237,I have implement this feature at repo cmdline,but it's a breeak implemention by extends from standard package flag. I have some further ideas on cli, but which maybe break v2 usage, so I tried to bring out my ideas at gxlb/cli . I found flags suits well of generic programming,so I tried to impliment it by my go generic tool gogp. Just as I have bring out, a flag have both LogicName and Name fields, If flag.Name is empty,then it is POSITIONAL-FLAG. Any more, I have designed Enums and Ranges to limit the valid values of a flag.

I'm trying to start this work base on cli v2 at this branch.

vipally avatar Apr 28 '21 02:04 vipally

Are there any updates on the progress of this feature? We eagerly await its arrival!

gaocegege avatar Dec 23 '22 07:12 gaocegege

It took 3 and a half years to implement the feature and it's still isn't finished or seems to be close to it. I don't know what the design of universal interface is but I think I know how to find the solution. Not sure whether it deserves own issue so I will post it here.

Solution

The solution is to turn the urfave/cli into nested arg parser and to let users bring their own parsers.

Motivation

The problem is that there is too much cases of how to implement arguments parsing because developers always can find new better way how to help user solve their problems. For example some of the programs which have non-trivial logic:

# [ - uses args as a language expression to return true or false
[ "a" == "b" ]
# expr - use args as a math formula to return result of calculation
expr 1 + 2
# bash uses separator to split own arguments from arguments of running script
bash -c 'echo $1' -- Hello!

This means that the solution development could be very long and probably unsuccessful. There is too much unknown options.

Design

I think it reasonable to let users bring their own parsers and after some time of real usage to choose the best to pack into the package itself or to use for new design. It could be achieved by creating new interface like ArgParser or so. This interface should provide methods to parse, provide usage string, help output, bash completions and other method required by urfave to connect sub parser seamlessly.

type ArgParser interface {
  // Parses args and returns a custom map filled with parsed values or an error 
  Parse(args []string) (cli.ArgsMap, error)
  // Returns a list of triplets: name, usage, short description
  GetUsage() [][]string
  // Prints help to output stream
  Help(args []string, w *io.Writer) error
  // Returns completion options
  Complete(args []string) ([]string, error)
}

Pros

  • It could be easily implemented in v2. Users would get ability to become involved into new parsers creation.
  • It unlocks the feature and untie implementation from the library design.
  • It gives almost limitless possibilities to implement custom parsers without the need to negotiate over design.
  • It makes parser composable and modular and let developers decide what they actually need.

Cons

  • It will make some cumbersome implementations to be hard to understand by developers without digging into the nested parser code.

teabroker avatar Oct 20 '23 14:10 teabroker