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

feat: parse default value with tag #73

Closed
wants to merge 1 commit into from
Closed

feat: parse default value with tag #73

wants to merge 1 commit into from

Conversation

wrfly
Copy link

@wrfly wrfly commented Mar 20, 2019

close #68

@coveralls
Copy link

coveralls commented Mar 20, 2019

Coverage Status

Coverage decreased (-0.6%) to 93.29% when pulling 918c260 on wrfly:master into 57836b8 on alexflint:master.

@alexflint
Copy link
Owner

Thanks for the PR @wrfly

But couldn't this be done by the user of this library? As in

var args struct {
	Foo string `default:"xyz"`
	Bar bool `default:"123"`
}
ecp.Default(&args)
arg.MustParse(&args)

I realize that this is an extra line (or three if you handle errors) on each usage of the library, but I think it's preferable to do it that way because it doesn't commit us to a particular defaults library, and users can therefore choose a different defaults library if they wish to.

It also leaves open the approach I currently recommended:

var args struct {
	Foo string
	Bar bool
	Baz MyStruct
}
args.Foo = "xyz"
args.Bar = true
args.Baz = MyStruct{ ... }
ecp.Default(&args)
arg.MustParse(&args)

This approach is a little less elegant, but it does work for user-defined structs such as MyStruct above.

@wrfly
Copy link
Author

wrfly commented Mar 21, 2019

@alexflint You are right, at the first beginning, I thought as you do, it's just a extra line, but somehow I thought it's kinda little ugly, so...
But since you said so, I'm OK with this. 😄
I'll close this PR and comment at #68

@wrfly wrfly closed this Mar 21, 2019
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.

Support for default field tag
3 participants