Introduce config.FromEnv() #41
Conversation
|
I'd like to see 0ebbcb6 as a PR from you with multiple commits into my branch. |
By implementing the encoding.TextUnmarshaler, Options can be populated by environment variables from env.
|
The diff itself now looks kinda good. Thanks! Would you mind start using this branch in one application and implement this change and creating a draft PR? Doing so allows testing this PR outside of tests and, if everything works, gives us more certainty before merging it and even tagging a release. |
|
The purpose of tests is exactly not needing what you suggest for more certainty. |
| // FromEnv parses environment variables and stores the result in the value pointed to by v. | ||
| // If v is nil or not a pointer, FromEnv returns an [ErrInvalidArgument] error. | ||
| func FromEnv(v Validator, options EnvOptions) error { | ||
| rv := reflect.ValueOf(v) |
There was a problem hiding this comment.
Since this is the third time we are using this check I would move that to a utility function validateNonNilPointer().
| return errors.Wrap(err, "can't parse environment variables") | ||
| } | ||
|
|
||
| return errors.Wrap(v.Validate(), "invalid configuration") |
There was a problem hiding this comment.
If I'm not mistaken, we've discussed in other PRs whether we should reduce error checks before return by directly returning errors.Wrap() (as it handles nil), but I'm not entirely sure we've agreed on doing that or not. Compared to the validate handling in FromYAMLFile(), the reduced version here gives the first impression that it will return an error.
I opt for not returning errors.Wrap() if the error could be nil as a rule.
There was a problem hiding this comment.
Personally, since I discovered that errors.Wrap() also handles nil errors, I do tend to use it as the last statement as well, instead of using an if statement and returning nil at the end. However, @oxzi also requested a change in #63 (comment) for not using it for readability reasons, so if you think this makes the code more readable/understandable, we can make it the standard.
There was a problem hiding this comment.
In the referenced comment, I primarily wanted a consistence between the two checks. Furthermore, when I am reading errors.Wrap(…), I am expecting an error to be handled and wrapped. Adding the if guard makes it more readable, imo. But honestly, I am fine with both.
| @@ -0,0 +1,101 @@ | |||
| package config | |||
FromEnv parses environment variables and stores the result in the value pointed to by v.
If v is nil or not a pointer, FromEnv returns an ErrInvalidArgument error.