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

Error aggregation #233

Merged
merged 2 commits into from Aug 22, 2022
Merged

Error aggregation #233

merged 2 commits into from Aug 22, 2022

Conversation

akutuev
Copy link
Contributor

@akutuev akutuev commented Aug 16, 2022

Hello,

The purpose of this PR is to implement Enhancement #208

This allows aggregating errors from parsing, so if you have multiple missing variables it prints like: "env: required environment variable "NAME" is not set; env: required environment variable "FRUIT" is not set".

The idea is to refactor a method doParse() by moving out all parsing process to a separate method doParseField(). In that case doParseField() may return an error per field hence doParse() may accumulate errors

Do let me know if any changes are required

@akutuev akutuev marked this pull request as ready for review August 16, 2022 12:36
@codecov
Copy link

codecov bot commented Aug 16, 2022

Codecov Report

Merging #233 (173496d) into main (4c93d81) will increase coverage by 0.87%.
The diff coverage is 96.66%.

❗ Current head 173496d differs from pull request most recent head 808b041. Consider uploading reports for the commit 808b041 to get more accurate results

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
+ Coverage   98.84%   99.71%   +0.87%     
==========================================
  Files           2        2              
  Lines         345      347       +2     
==========================================
+ Hits          341      346       +5     
+ Misses          4        1       -3     
Impacted Files Coverage Δ
env.go 99.70% <96.66%> (+0.88%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@caarlos0
Copy link
Owner

I think its a little weird that it prints the env: suffix so many times, maybe its worth revisiting where errors are returned and add the env prefix only once? wdyt?

@akutuev
Copy link
Contributor Author

akutuev commented Aug 16, 2022

@caarlos0 I think this is a good idea, since we have a full control over all errors we can aggregate it nicely, let me try to improve this

@akutuev
Copy link
Contributor Author

akutuev commented Aug 19, 2022

@caarlos0, updated PR as discussed.
Added aggregateError struct to be able to aggregate all errors and print "env" as a prefix only once

@caarlos0 caarlos0 merged commit 69c7b5a into caarlos0:main Aug 22, 2022
@caarlos0
Copy link
Owner

lg, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants