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

Add command line flag support #328

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ryanfaerman
Copy link
Contributor

This builds on the work that was recently merged, adding argument support. I've extended it a bit, adding support for optional flags in addition to positional arguments.

Given the following function in a magefile:

func Say(animal string, loud mg.BoolFlag, msgs mg.StringSliceFlag) {
        fmt.Println("what does the", animal, "say?")
	if loud {
		fmt.Println("GET LOUD")
	}

	for _, m := range msgs {
		if loud {
			m = strings.ToUpper(m)
		}
		fmt.Println(m)
	}
}

It can now be executed as any of the following:

mage say fox --loud --msgs="bark bark" --msgs="woof woof" --msgs="yip yip"
mage say fox --msgs="bark bark" --msgs="woof woof" --msgs="yip yip"
mage say --msgs="bark bark" --msgs="woof woof" --msgs="yip yip" fox
mage say fox --msgs="bark bark"

Ryan Faerman added 3 commits December 30, 2020 23:23
Flags are optional whereas positional arguments are required
Otherwise, non-string flags would fail to exec properly when they are
blank. This ensures we always have a convertable value
}

// ZeroValue returns code needed to set the default value for a flag when it is
// undefined. It needs to be a value that can be converted to the correct tyoe
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Typo on the work 'type', on line 70

@natefinch
Copy link
Member

So the flags become optional parameters that you need to use -- to specify? That's interesting... but I'm not 100% sure the extra complexity in the code and the docs is worth it. Lemme think about it.

@arschles
Copy link
Contributor

arschles commented Oct 1, 2021

@natefinch FWIW, I do think this is worthwhile. having named arguments is often more clear to users because they're more explicit. I've also had need for optional parameters to a target that I had to use environment variables for, and that's less discoverable for users.

@sheldonhull
Copy link

I'll add a quick note to agree with @arschles.
The one thing that I was really surprised at and actually made things initially more complicated in adopting mage was lack of standard flag handling. I'd be fine with param=1 mage do or sticking with standard mage do -filter "foo" as well. I have tried to avoid parameters for any tasks where possible because it seems I can't make them optional right now (maybe I'm wrong there), and it's confusing to type parameters as if they are commands.

If possible, I'd wish to support single dash flags since that's so common in Go, but double dash is also great.


commands := []string{}

Loop:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a goto in the wild, like finding a yeti 😆

}

// ZeroValue returns code needed to set the default value for a flag when it is
// undefined. It needs to be a value that can be converted to the correct tyoe

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// undefined. It needs to be a value that can be converted to the correct tyoe
// undefined. It needs to be a value that can be converted to the correct typo

@giladreich
Copy link

giladreich commented Oct 31, 2021

@ryanfaerman, thank you so much for this.

@natefinch 👋 , can we please consider to add this? I've read through different threads (e.g. #24) and to me it feels as this is the right approach, given that Golang doesn't support overloading methods or optional arguments.

At the time of writing I have to do this:

func Test() error {
	mg.Deps(Prep)
	pkgNames := os.Getenv("PKG")
	filterRegex := os.Getenv("FILTER")
	args := []string{"test", "-v"}
	if len(pkgNames) != 0 {
		for _, pkgName := range strings.Split(pkgNames, ",") {
			args = append(args, "./"+pkgName)
		}
	} else {
		args = append(args, "./...")
	}

	if len(filterRegex) != 0 {
		args = append(args, "-run", filterRegex)
	}

	return sh.Run("go", args...)
}

And then I can invoke the following ways:

// All cases
mage test

// Single package
PKG="util" mage test

// Multiple packages
PKG="util,crypt" mage test

// Multiple packages with filter
PKG="util,crypt" FILTER="Test*" mage test

Doesn't feel as intuitive or perhaps it's just me doing it wrong.

@JeanChristopheMorinPerso

Hello, just stopping by to say that I would be interested by this PR! The main advantage to me is the discoverability of target options/flags and better support for Windows (where SOMETHING=asd mage ... is not valid).

@natefinch
Copy link
Member

I definitely see the value in this, but I think I want to go a slightly different way.

I think we could dispense with mg.BoolFlag and just use pointers (*bool etc) instead. I think that's obvious enough to a go developer that *bool might be true, false, or unset.

@c0nfleis
Copy link

What's the status on this? We build internal tooling using mage, and we have had to use a plethora of ENV vars in place of flags, which can be very convoluted at times :/

@natefinch
Copy link
Member

I'll spend some time on it this weekend, see if I can knock it out. I know it would be very useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants