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

feat: wrap external errors instead of changing them to root errors #84

Merged
merged 1 commit into from
Apr 28, 2020

Conversation

morningvera
Copy link
Member

@morningvera morningvera commented Apr 21, 2020

this PR is a redesign of the way eris wraps external errors. instead of unwrapping them and disguising them as root errors, it simply wraps them to maintain all error context. closes #82 and closes #81.

@morningvera morningvera added the enhancement New feature or request label Apr 21, 2020
@morningvera morningvera requested a review from sum2000 April 21, 2020 02:18
@morningvera
Copy link
Member Author

morningvera commented Apr 23, 2020

I added a test using the Golang As and confirmed that it works now. We can add our own version of it in another PR.

Copy link
Member Author

@morningvera morningvera left a comment

Choose a reason for hiding this comment

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

Reminder to update the readme to reflect external error handling changes

Copy link
Member

@sum2000 sum2000 left a comment

Choose a reason for hiding this comment

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

we may need to modify/add examples

eris.go Show resolved Hide resolved
format.go Show resolved Hide resolved
eris_test.go Show resolved Hide resolved
eris_test.go Show resolved Hide resolved
eris_test.go Outdated Show resolved Hide resolved
@sum2000
Copy link
Member

sum2000 commented Apr 26, 2020

Since we are adding external error in root error, the New() method should be capable of the same. I should be able to create a new root error from external error. what do you think?

@sum2000
Copy link
Member

sum2000 commented Apr 26, 2020

Reviewed the code, reviewing the tests locally

@morningvera
Copy link
Member Author

Since we are adding external error in root error, the New() method should be capable of the same. I should be able to create a new root error from external error. what do you think?

wouldn't that be equivalent to wrapping an external error? i think the method definition would look something like NewWithExt(ext error, msg string), which is the same definition and implementation of our wrap methods so this use case is already covered

@sum2000
Copy link
Member

sum2000 commented Apr 28, 2020

Since we are adding external error in root error, the New() method should be capable of the same. I should be able to create a new root error from external error. what do you think?

wouldn't that be equivalent to wrapping an external error? i think the method definition would look something like NewWithExt(ext error, msg string), which is the same definition and implementation of our wrap methods so this use case is already covered

It won't be equivalent to wrapping because the wrap method returns wrap error type and the proposed method would return root error type

@morningvera
Copy link
Member Author

that's actually incorrect. for the external error case, wrap returns a root error.

https://github.com/rotisserie/eris/pull/84/files#diff-0bccded79c042f8676aa4c5a07863306R78

@codecov-io
Copy link

Codecov Report

Merging #84 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
- Coverage   98.91%   98.90%   -0.02%     
==========================================
  Files           3        3              
  Lines         277      274       -3     
==========================================
- Hits          274      271       -3     
  Misses          1        1              
  Partials        2        2              
Impacted Files Coverage Δ
eris.go 100.00% <100.00%> (ø)
format.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f16804a...49834ba. Read the comment docs.

@morningvera morningvera merged commit 2d302b7 into master Apr 28, 2020
@morningvera morningvera deleted the feat/ext-errors branch May 5, 2020 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants