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 default values #8

Merged
merged 3 commits into from
Aug 3, 2020
Merged

Conversation

jimmykodes
Copy link
Contributor

This will allow specifying a default value to be used in the case that none of the key options are found in the env.

jimmykodes added 2 commits July 30, 2020 15:31
this will split a tag on a || and use the value following the pipes
(if any) as a default value if none of the env var options return
a value
@hinshun
Copy link
Contributor

hinshun commented Jul 31, 2020

Hi @jimmykodes, thanks for the PR!

I see that you decided to pick || as the sentinel value to define the default value. Looking around at other struct tags for ideas, it looks like json uses commas to delimit options like omitempty. e.g. json:"myField,omitempty"

Could you change this so it follows this convention, and allows more options in the future? I'd like to see: env:"MY_ENV,default=myValue"

@hinshun
Copy link
Contributor

hinshun commented Jul 31, 2020

Also looks pretty similar to: #5

@jimmykodes
Copy link
Contributor Author

@hinshun Thanks for the feedback!

I've updated things to use default= instead of ||. and, as you pointed out the similarity w/ #5, i've opted to include the required field here as well.

I've chosen to handle required by using required=true|false I understand this is a diversion from something like json:"omitempty" but this implementation allowed it to fit nicely in my switch statement and i also felt doing something like env:"required" would be ambiguous as to whether the field should be required or if that is the name of the env var to look for. Hope that all makes sense.

While i'm here, if you have any other extra tag behaviors you'd like to see added, i'd be happy to take a crack at them.

Copy link
Contributor

@hinshun hinshun left a comment

Choose a reason for hiding this comment

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

Looks very close 👍. Actually, for json struct tags json:",omitempty" is how they disambiguate between the field name and options. I would prefer env:",required" to match this convention.

@jimmykodes
Copy link
Contributor Author

That solves the ambiguity for a single value, but does not solve a case like this: env:"REQUIRED,required" where required could be a key to lookup or marking the field required. If you consider this ambiguity to be acceptable, let me know and i'll get that updated.

A possible alternate solution i see would be to structure tags like env:all,the,env,keys;default=val,required where all the possible env var keys are on the left of the semicolon and all option arguments would be on the right.

either way, lemme know what you would prefer.

@hinshun
Copy link
Contributor

hinshun commented Aug 3, 2020

You're right, I haven't thought of the multiple env keys. Let's just stick with your current implementation then.

@hinshun hinshun merged commit 9271595 into Netflix:master Aug 3, 2020
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.

2 participants