magefile / mage

a Make/rake-like dev tool using Go
https://magefile.org
Apache License 2.0
4.01k stars 250 forks source link

Enhancement: Non Mandatory Arguments/Flags #397

Open DeadlySurgeon opened 2 years ago

DeadlySurgeon commented 2 years ago

It would be nice if arguments aren't required. I could see several reasons for wanting defaults be in. Obviously the issue with how things are now is that the arguments are order based not named based so with how mage is now I doubt that could be supported. But what if arguments could be passed in like flags, that would then be checked against the function's parameters? Using AST or Reflect you can see the function's parameters names, and match the inputing flags to the names.

func Example(key1 string, key2 string, key3 string) { ... }

From there, --key1=abc --key2=dce or whatever else could be for mounting. Or, if no keys are specified then orders would take over.

Or better yet, just allow os.Args to be referenced so we can do the parsing ourselves. Mandatory arguments are problematic enough that it makes them not worth using, at least to me, and the convenience of automatically parsed fields does not make up for it.

perj commented 2 years ago

So at the risk of becoming the "variadic" guy now, I think that's the obvious solution here. That if the last argument of the function is ...string then any remaining arguments are passed through to it. The function can then create a flagset and parse it if that's what desired.

Disadvantage of that would be that it's not visible in mage -h target. Some way of extending the help for a specific target could be enough for that though.

If mage really needs to know about what flags a target has I think a flagset has to be declared outside of the function somehow, perhaps including a usage function.

In that case I can imagine something like

var buildNocache = mg.FlagsFor(Build).Bool("nocache", false, "Ignore cache.")

func init() {
    mg.FlagsFor(Build).Usage = func() {
        fmt.Fprintf(os.Stderr, "Usage: mage build [options] target\n")
    }
}

func Build(target string) {
    if *buildNocache {
        ...
    }
    ...
}

A more complicated solution but doable. To make the target also option, both solutions could be combined.

Just some thoughts, let me know what you think.

thorntonrose commented 9 months ago

I suggest adding a flag for specifying key/value pairs (properties).

Example CLI usage:

mage -p package=./foo -p tests=TestBar test

Example Magefile usage:

func Test() {
   pkg := mg.Property("package", "./...") // 2nd parameter is default value
   tests := mg.Property("tests", "Test")
   // ...
}

Many other build tools do this.

thorntonrose commented 9 months ago

Mandatory arguments are problematic enough that it makes them not worth using, at least to me, and the convenience of automatically parsed fields does not make up for it.

I agree. When I saw that target arguments are mandatory, I ditched them. For now, I use env vars.

CLI example:

PACKAGE=./foo TESTS=TestBar mage -v test

Magefile example:

func getenv(key string, def string) string {
   v := os.Getenv(key)

   if v == "" {
      v = def
   }

   return v
}

func Test() {
   pkg := getenv("PACKAGE", "./...")
   tests := getenv("TESTS", "Test")
   // ...
}
Alepernicolo commented 7 months ago

It would be great if variadic parameters were supported for mage targets