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

Args (an alternative to Flags) #1074

Closed
3 tasks done
Nokel81 opened this issue Feb 24, 2020 · 24 comments · Fixed by #1833
Closed
3 tasks done

Args (an alternative to Flags) #1074

Nokel81 opened this issue Feb 24, 2020 · 24 comments · Fixed by #1833
Labels
area/v3 relates to / is being considered for v3 help wanted please help if you can! kind/feature describes a code enhancement / feature request status/confirmed confirmed to be valid, but work has yet to start
Milestone

Comments

@Nokel81
Copy link

Nokel81 commented Feb 24, 2020

Checklist

  • Are you running the latest v2 release? The list of releases is here.
  • Did you check the manual for your release? The v2 manual is here
  • 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 Nokel81 added area/v2 relates to / is being considered for v2 status/triage maintainers still need to look into this labels Feb 24, 2020
@coilysiren
Copy link
Member

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 👍

@saschagrunert
Copy link
Member

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{}

@Nokel81
Copy link
Author

Nokel81 commented Feb 25, 2020

  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)

@coilysiren coilysiren added status/confirmed confirmed to be valid, but work has yet to start help wanted please help if you can! and removed status/triage maintainers still need to look into this labels Feb 26, 2020
@coilysiren
Copy link
Member

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

@costela
Copy link

costela commented Feb 26, 2020

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. 🤔

@Nokel81
Copy link
Author

Nokel81 commented Mar 5, 2020

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

@coilysiren
Copy link
Member

Whatever we do for this feature request (eg. #1074), it should also most likely solve this request too => #1069

@Nokel81
Copy link
Author

Nokel81 commented Mar 31, 2020

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

@stale
Copy link

stale bot commented Jun 30, 2020

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 stale bot added the status/stale stale due to the age of it's last update label Jun 30, 2020
@coilysiren
Copy link
Member

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

@stale
Copy link

stale bot commented Jul 2, 2020

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.

@coilysiren coilysiren removed the status/stale stale due to the age of it's last update label Jul 2, 2020
@clarktlaugh
Copy link

I would really like to see this too

@stale
Copy link

stale bot commented Nov 2, 2020

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 stale bot added the status/stale stale due to the age of it's last update label Nov 2, 2020
@stale
Copy link

stale bot commented Dec 3, 2020

Closing this as it has become stale.

@stale stale bot closed this as completed Dec 3, 2020
@coilysiren
Copy link
Member

👀

@coilysiren coilysiren reopened this Jan 15, 2021
@stale
Copy link

stale bot commented Jan 15, 2021

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 stale bot removed the status/stale stale due to the age of it's last update label Jan 15, 2021
@coilysiren
Copy link
Member

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

@dearchap
Copy link
Contributor

dearchap commented Feb 8, 2021

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
Copy link
Contributor

dearchap commented Mar 21, 2021

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)

}

@coilysiren
Copy link
Member

Random FYI, people have since created #1237 and #1246 which as describing essentially the same functionality that's described here

@coilysiren
Copy link
Member

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 🙏🏼

@vipally
Copy link
Contributor

vipally commented Apr 28, 2021

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.

@meatballhat meatballhat added the kind/feature describes a code enhancement / feature request label Apr 21, 2022
@meatballhat meatballhat changed the title v2 feature: Args (an alternative to Flags) Args (an alternative to Flags) Apr 21, 2022
@dearchap dearchap added area/v3 relates to / is being considered for v3 and removed area/v2 relates to / is being considered for v2 labels Oct 31, 2022
@gaocegege
Copy link

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

@teabroker
Copy link

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.

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 help wanted please help if you can! kind/feature describes a code enhancement / feature request status/confirmed confirmed to be valid, but work has yet to start
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants