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

Extend HTTPError to satisfy the Go 1.13 error wrapper interface #1559

Merged
merged 2 commits into from
Jul 23, 2020
Merged

Extend HTTPError to satisfy the Go 1.13 error wrapper interface #1559

merged 2 commits into from
Jul 23, 2020

Conversation

flimzy
Copy link
Contributor

@flimzy flimzy commented Apr 29, 2020

I don't see that this has been discussed before in the issues, so my apologies if this has been discussed and rejected for some reason.

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #1559 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
+ Coverage   84.94%   84.96%   +0.01%     
==========================================
  Files          28       28              
  Lines        2166     2168       +2     
==========================================
+ Hits         1840     1842       +2     
  Misses        211      211              
  Partials      115      115              
Impacted Files Coverage Δ
echo.go 85.93% <100.00%> (+0.08%) ⬆️

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 c08f303...ea34bf9. Read the comment docs.

@flimzy
Copy link
Contributor Author

flimzy commented Apr 29, 2020

- Misses        211      213       +2     

I'm happy to add a test for this if it's deemed appropriate.

@lammel
Copy link
Contributor

lammel commented May 5, 2020

@flimzy Could you please add a test for unwrap. This will make coverage happy and ensure it will be kept in the future.

Thanks for the contribution.

@flimzy
Copy link
Contributor Author

flimzy commented May 5, 2020

@flimzy Could you please add a test for unwrap. This will make coverage happy and ensure it will be kept in the future.

Yep, no problem.

I've pushed an additional commit with tests. Let me know if I can change anything else here.

Note: The tests only run in Go 1.13 or later. I could run tests on older versions of Go using the golang.org/x/xerrors package, but unless you think that's especially valuable, I didn't want to add another dependency just for a simple test.

@lammel
Copy link
Contributor

lammel commented May 7, 2020

Referencing #1472 as a duplicate.

This PR looks clean for building also on older versions of go

@flimzy
Copy link
Contributor Author

flimzy commented May 7, 2020

That PR's test doesn't directly validate that the error works with Go 1.13, rather it infers it. But I think that's acceptable, since the correctness can be easily validated visually.

@ffenix113
Copy link
Contributor

Is there any ETA on when(if) this would be merged?

@lammel lammel merged commit d324506 into labstack:master Jul 23, 2020
@flimzy flimzy deleted the wrapper branch July 23, 2020 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants