-
Notifications
You must be signed in to change notification settings - Fork 215
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
Wl/config url #181
Wl/config url #181
Conversation
Codecov Report
@@ Coverage Diff @@
## master #181 +/- ##
==========================================
+ Coverage 84.12% 85.01% +0.89%
==========================================
Files 9 9
Lines 529 534 +5
==========================================
+ Hits 445 454 +9
+ Misses 65 58 -7
- Partials 19 22 +3
Continue to review full report at Codecov.
|
Looks great! Just one change re: error handling and I think this is good to merge. Bonus: you can try adding another test that doesn't start an HTTP server (so that the HTTP request fails), and check to make sure you get the right error message. That would catch the current bug. |
…o Http server is configured
… into wl/config-URL
pkg/config/config.go
Outdated
} | ||
if err != nil { | ||
return Configuration{}, err | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still has to go outside the else
statement, or the err
on line 131 won't get caught
pkg/config/config.go
Outdated
if path == "" { | ||
rawBytes, err = configBox.Find("config.yaml") | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you move the if err != nil
below, this check can go away, since it'll be handled outside the if/else
.
Looking good! Just one more small change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Will!
Expanded ParseFile to include URL's and added a test for it.