-
Notifications
You must be signed in to change notification settings - Fork 2.2k
refactor: Use errors
package to compare and assert error types
#3739
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
Conversation
Actually I added a lint rule go-errorlint to do it, would you like to add it to this project? If so I could open another pr to do it because I think it's better to leave 2 commit for |
That sounds great, thank you, @zyfy29! |
Also, please fix the broken tests and linter errors. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3739 +/- ##
=======================================
Coverage 91.11% 91.11%
=======================================
Files 187 187
Lines 16697 16702 +5
=======================================
+ Hits 15213 15218 +5
Misses 1296 1296
Partials 188 188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thank you, @zyfy29! I like this much better.
Just a few more questions and suggestions for you, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging since many things have changed from the first review.
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.
Sorry for the mistakes, I found most of them are made by Intellij IDEA's quick-fix and now I've checked it again.
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.
Thank you, @zyfy29!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging (since many changes have occurred since first approval).
cc: @stevehipwell - @alexandear
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.
LGTM
Thank you, @stevehipwell! @zyfy29 - please follow up with a PR that enables linting for the usage of the |
close #3699
follows https://go.dev/blog/go1.13-errors to handle error types with
errors
package