Introduce config.FromEnv() #41
Conversation
|
I'd like to see 0ebbcb6 as a PR from you with multiple commits into my branch. |
|
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. |
config/config.go
Outdated
| 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.
There was a problem hiding this comment.
Ok, I'd like to define this now: Don't return errors.Wrap() directly if the error could be nil.
config/config_test.go
Outdated
| @@ -0,0 +1,101 @@ | |||
| package config | |||
There was a problem hiding this comment.
@Al2Klimov Please only fix the direct errors.Wrap() return. Tests will be adjusted with #77.
config/config.go
Outdated
| return errors.Wrap(err, "can't parse environment variables") | ||
| } | ||
|
|
||
| return errors.Wrap(v.Validate(), "invalid configuration") |
There was a problem hiding this comment.
Ok, I'd like to define this now: Don't return errors.Wrap() directly if the error could be nil.
By implementing the encoding.TextUnmarshaler, Options can be populated by environment variables from env.
Test everything related to config
I already fixed the direct return of |
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.