-
Notifications
You must be signed in to change notification settings - Fork 413
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
Use errors and fmt package to join and wrap errors #460
Conversation
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.
A few suggestions.
But don't get me wrong about the number of comments I posted, I love your changes
"flag" | ||
"fmt" | ||
"io/ioutil" |
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.
Side remark as you are upgrading code to use go 1.13 wrapping
io/ioutil is deprecate
Maybe something could be done too. Not necessarily in this PR, it's up to you
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.
Good spot.
I added the replacements in 3dc9d72
components/cqrs/command_bus.go
Outdated
"errors" | ||
stdErrors "errors" |
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.
Nope, we are importing twice.
It's a side effect of previous usage of two packages for errors. But now, you should clean
Please replace all the stdErrors.
to errors.Is
components/cqrs/command_processor.go
Outdated
@@ -1,11 +1,10 @@ | |||
package cqrs | |||
|
|||
import ( | |||
"errors" | |||
stdErrors "errors" |
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.
see #460 (comment)
components/cqrs/event_bus.go
Outdated
@@ -2,11 +2,12 @@ package cqrs | |||
|
|||
import ( | |||
"context" | |||
"errors" | |||
stdErrors "errors" |
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.
Same
components/cqrs/event_bus_test.go
Outdated
"fmt" | ||
"testing" | ||
|
||
"errors" | ||
"github.com/ThreeDotsLabs/watermill/components/cqrs" |
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.
"fmt" | |
"testing" | |
"errors" | |
"github.com/ThreeDotsLabs/watermill/components/cqrs" | |
"fmt" | |
"errors" | |
"testing" | |
"github.com/ThreeDotsLabs/watermill/components/cqrs" |
@@ -9,8 +9,8 @@ import ( | |||
|
|||
"github.com/stretchr/testify/assert" | |||
|
|||
"errors" |
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.
Same
@@ -5,7 +5,7 @@ import ( | |||
"testing" | |||
"time" | |||
|
|||
"github.com/pkg/errors" | |||
"errors" |
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.
Same
message/router_test.go
Outdated
@@ -7,7 +7,7 @@ import ( | |||
"testing" | |||
"time" | |||
|
|||
"github.com/pkg/errors" | |||
"errors" |
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.
Same
pubsub/gochannel/pubsub.go
Outdated
@@ -4,8 +4,8 @@ import ( | |||
"context" | |||
"sync" | |||
|
|||
"errors" |
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.
Same
As of go 1.20 the go `errors` package supports `errors.Join` which replaces the need for the `hashicorp/go-multierror` package which was providing the same functionality. To wrap errors `fmt.Errorf` with the `%w` format specifier (introduced in golang 1.13) is now used instead of `errors.Wrap` which was provided by the `github.com/pkg/errors` package.
Remove the usage of ioutil which was deprecated in golang 1.16 See https://go.dev/doc/go1.16#ioutil
Hey @ccoVeille Thank you for the code review. Let me know if I missed something or if some other change is needed. |
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" |
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.
I think this block should be before the watermill ones
components/metrics/http_test.go
Outdated
if err != nil { | ||
t.Fatal(errors.Wrap(err, "call to unknown endpoint failed")) | ||
t.Fatal(fmt.Errorf("call to unknown endpoint failed: %w", err)) | ||
} |
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.
Not related to your changes
But here I would expect to have
require.NoError(t, err, "call to unknown failed")
components/metrics/http_test.go
Outdated
@@ -46,7 +46,7 @@ func TestCreateRegistryAndServeHTTP_unknown_endpoint(t *testing.T) { | |||
} | |||
|
|||
if err != nil { | |||
t.Fatal(errors.Wrap(err, "call to unknown endpoint failed")) | |||
t.Fatal(fmt.Errorf("call to unknown endpoint failed: %w", err)) |
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 is a fatalf
t.Fatal(fmt.Errorf("call to unknown endpoint failed: %w", err)) | |
t.Fatalf("call to unknown endpoint failed: %v", err) |
components/metrics/http_test.go
Outdated
t.Fatal(errors.Wrap(err, "registration of prometheus build info collector failed")) | ||
t.Fatal(fmt.Errorf("registration of prometheus build info collector failed: %w", err)) |
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.
Here the previous code is somehow bad.
And you are changing it to something better, but unfortunately still bad.
See
components/metrics/http_test.go
Outdated
if err != nil { | ||
t.Fatal(errors.Wrap(err, "call to metrics endpoint failed")) | ||
t.Fatal(fmt.Errorf("call to metrics endpoint failed: %w", err)) | ||
} |
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.
Same either Fatalf, or require.NoError
Hey @ccoVeille I added commit bbe870c to remove some Please have another look and let me know if I missed something (again) 😄 |
I can help with adding golangci-lint checkers to avoid such issues and others |
I opened #463 about it |
@@ -26,7 +25,7 @@ func Recoverer(h message.HandlerFunc) message.HandlerFunc { | |||
|
|||
defer func() { | |||
if r := recover(); r != nil || panicked { | |||
err = errors.WithStack(RecoveredPanicError{V: r, Stacktrace: string(debug.Stack())}) |
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 might be a bit of a breaking change, actually. 🤔 Perhaps we could somehow keep the stack trace here?
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.
I'm not sure how this really is a breaking change or not.
A debug.Stack
call is used to get the stacktrace into RecoveredPanicError
. (via a call to runtime.Callers
).
This seems to me like the same function that errors.WithStack
provides as it's using runtime.Callers
and then formatting it.
Now, should someone use reflection or have an interface to represent and error from the errors package then yes this PR could break things.
Personally, I feel this change is not breaking the contract of the library as it's still returning the error
interface.
I'm unsure why the PR was closed. The code was OK. Is it because of the comment here? This was a small part of your changes. Even if there were a discussion about it, most of the code was OK to merge, no? |
@ccoVeille I agree. I'm not sure about the comment yet (I'm reviewing lots of Watermill PRs now, so I need some time), but the rest of the good looks good to me. |
Oh, I think the OP's account got deleted. |
Oh OK. Then you can consider to reopen the PR. I can do it of course, but with some delay maybe. I hope the commits will stay live for a few days |
Sadly, it seems we can't reopen it, as the fork is gone. 🫨 |
Are you able to merge? You could revert then. Quite dirty, but why not |
You can use this as a maintainer maybe https://github.blog/news-insights/product-news/change-the-base-branch-of-a-pull-request/ this would prefer to merge in master, you will have then a branch and you will be able to open a PR for it |
Good day!
First of thank you for maintaining and providing the watermill package!
I opened this PR because I noticed that the
hashicorp/go-multierror
andgithub.com/pkg/errors
where added as an indirect dependency when using watermill.As these dependency have native alternatives I created this PR.
Since golang 1.20 the go
errors
package supportserrors.Join
which replaces the need for thehashicorp/go-multierror
package which was providing the same functionality.To wrap errors
fmt.Errorf
with the%w
format specifier (introduced in golang 1.13) is now used instead oferrors.Wrap
which was provided by thegithub.com/pkg/errors
package.Please let me know what you think and if any changes are needed.
Have a great day and thank you for reviewing this PR!
Cheers,
Warnar
This PR resolves #447