Use errors and fmt package to join and wrap errors #460

ghost · 2024-07-31T10:05:26Z

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 and github.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 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.

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

ccoVeille

A few suggestions.

But don't get me wrong about the number of comments I posted, I love your changes

ccoVeille · 2024-08-01T11:50:45Z

_examples/real-world-examples/receiving-webhooks/main.go

 	"flag"
+	"fmt"
 	"io/ioutil"


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

Good spot.
I added the replacements in 3dc9d72

ccoVeille · 2024-08-01T11:52:44Z

components/cqrs/command_bus.go

+	"errors"
 	stdErrors "errors"


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

ccoVeille · 2024-08-01T11:54:34Z

components/cqrs/command_processor.go

@@ -1,11 +1,10 @@
 package cqrs

 import (
+	"errors"
 	stdErrors "errors"


see #460 (comment)

ccoVeille · 2024-08-01T12:13:02Z

components/cqrs/event_bus.go

@@ -2,11 +2,12 @@ package cqrs

 import (
 	"context"
+	"errors"
 	stdErrors "errors"


ccoVeille · 2024-08-01T12:13:50Z

components/cqrs/event_bus_test.go

 	"fmt"
 	"testing"

+	"errors"
 	"github.com/ThreeDotsLabs/watermill/components/cqrs"


Suggested change

"fmt"

"testing"

"errors"

"github.com/ThreeDotsLabs/watermill/components/cqrs"

"fmt"

"errors"

"testing"

"github.com/ThreeDotsLabs/watermill/components/cqrs"

ccoVeille · 2024-08-01T12:22:17Z

message/router/middleware/retry_test.go

@@ -9,8 +9,8 @@ import (

 	"github.com/stretchr/testify/assert"

+	"errors"


ccoVeille · 2024-08-01T12:22:25Z

message/router/middleware/throttle_test.go

@@ -5,7 +5,7 @@ import (
 	"testing"
 	"time"

-	"github.com/pkg/errors"
+	"errors"


ccoVeille · 2024-08-01T12:22:33Z

message/router_test.go

@@ -7,7 +7,7 @@ import (
 	"testing"
 	"time"

-	"github.com/pkg/errors"
+	"errors"


ccoVeille · 2024-08-01T12:22:41Z

pubsub/gochannel/pubsub.go

@@ -4,8 +4,8 @@ import (
 	"context"
 	"sync"

+	"errors"


tools/mill/cmd/googlecloud.go

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

ghost · 2024-08-01T15:24:28Z

Hey @ccoVeille

Thank you for the code review.
I think I managed to sort all the imports correctly now and also removed ioutil usage.

Let me know if I missed something or if some other change is needed.
Cheers

ccoVeille · 2024-08-01T16:53:27Z

message/router/middleware/poison_test.go

 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"


I think this block should be before the watermill ones

message/router/middleware/message_test.go

ccoVeille · 2024-08-01T16:58:22Z

components/metrics/http_test.go

 	if err != nil {
-		t.Fatal(errors.Wrap(err, "call to unknown endpoint failed"))
+		t.Fatal(fmt.Errorf("call to unknown endpoint failed: %w", err))
 	}


Not related to your changes

But here I would expect to have

require.NoError(t, err, "call to unknown failed")

ccoVeille · 2024-08-01T16:58:57Z

components/metrics/http_test.go

@@ -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))


This is a fatalf

Suggested change

t.Fatal(fmt.Errorf("call to unknown endpoint failed: %w", err))

t.Fatalf("call to unknown endpoint failed: %v", err)

ccoVeille · 2024-08-01T17:05:36Z

components/metrics/http_test.go

-		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))


Here the previous code is somehow bad.

And you are changing it to something better, but unfortunately still bad.

See

ccoVeille · 2024-08-01T17:06:08Z

components/metrics/http_test.go

 	if err != nil {
-		t.Fatal(errors.Wrap(err, "call to metrics endpoint failed"))
+		t.Fatal(fmt.Errorf("call to metrics endpoint failed: %w", err))
 	}


Same either Fatalf, or require.NoError

ghost · 2024-08-02T07:52:41Z

Hey @ccoVeille

I added commit bbe870c to remove some t.Fatal and replaced them with testify require.
(I hope) I also fixed all import sort order issues now.

Please have another look and let me know if I missed something (again) 😄

ccoVeille · 2024-08-02T12:25:20Z

I can help with adding golangci-lint checkers to avoid such issues and others

ccoVeille · 2024-08-03T12:32:28Z

I can help with adding golangci-lint checkers to avoid such issues and others

I opened #463 about it

m110 · 2024-08-29T07:20:17Z

message/router/middleware/recoverer.go

@@ -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())})


This might be a bit of a breaking change, actually. 🤔 Perhaps we could somehow keep the stack trace here?

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.

ccoVeille · 2024-09-01T19:33:41Z

I'm unsure why the PR was closed.

The code was OK. Is it because of the comment here?

#460 (comment)

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?

m110 · 2024-09-01T20:13:15Z

@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.

m110 · 2024-09-01T20:14:19Z

Oh, I think the OP's account got deleted.

ccoVeille · 2024-09-01T20:15:57Z

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

m110 · 2024-09-01T20:39:40Z

Sadly, it seems we can't reopen it, as the fork is gone. 🫨

ccoVeille · 2024-09-01T20:50:38Z

Are you able to merge? You could revert then.

Quite dirty, but why not

ccoVeille · 2024-09-01T21:29:13Z

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

ccoVeille reviewed Aug 1, 2024

View reviewed changes

ghost force-pushed the native-errors branch from f81e029 to ecb2a29 Compare August 1, 2024 15:16

Warnar Boekkooi added 2 commits August 1, 2024 17:22

Replace deprecated io/ioutil calls

3dc9d72

Remove the usage of ioutil which was deprecated in golang 1.16 See https://go.dev/doc/go1.16#ioutil

ghost force-pushed the native-errors branch from ecb2a29 to 3dc9d72 Compare August 1, 2024 15:22

ccoVeille reviewed Aug 1, 2024

View reviewed changes

Replace t.Fatal with testify/require

bbe870c

ccoVeille approved these changes Aug 2, 2024

View reviewed changes

ghost mentioned this pull request Aug 2, 2024

Use errors and fmt package to wrap errors ThreeDotsLabs/watermill-sql#33

Merged

ccoVeille mentioned this pull request Aug 3, 2024

I can help with adding golangci-lint checkers to avoid issues #463

Open

Merge branch 'master' into native-errors

9811535

m110 reviewed Aug 29, 2024

View reviewed changes

ghost closed this by deleting the head repository Aug 29, 2024

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use errors and fmt package to join and wrap errors #460

Use errors and fmt package to join and wrap errors #460

ghost commented Jul 31, 2024 •

edited by ghost

Loading

ccoVeille left a comment

ccoVeille Aug 1, 2024

ghost Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ghost commented Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ccoVeille Aug 1, 2024

ghost commented Aug 2, 2024

ccoVeille commented Aug 2, 2024

ccoVeille commented Aug 3, 2024

m110 Aug 29, 2024

ghost Aug 29, 2024

ccoVeille commented Sep 1, 2024

m110 commented Sep 1, 2024

m110 commented Sep 1, 2024

ccoVeille commented Sep 1, 2024

m110 commented Sep 1, 2024

ccoVeille commented Sep 1, 2024

ccoVeille commented Sep 1, 2024

		@@ -9,8 +9,8 @@ import (

		"github.com/stretchr/testify/assert"

		"errors"

		"github.com/stretchr/testify/assert"
		"github.com/stretchr/testify/require"

	t.Fatal(fmt.Errorf("call to unknown endpoint failed: %w", err))
	t.Fatalf("call to unknown endpoint failed: %v", err)

		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))

Use errors and fmt package to join and wrap errors #460

Use errors and fmt package to join and wrap errors #460

Conversation

ghost commented Jul 31, 2024 • edited by ghost Loading

ccoVeille left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 2, 2024

ccoVeille commented Aug 2, 2024

ccoVeille commented Aug 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccoVeille commented Sep 1, 2024

m110 commented Sep 1, 2024

m110 commented Sep 1, 2024

ccoVeille commented Sep 1, 2024

m110 commented Sep 1, 2024

ccoVeille commented Sep 1, 2024

ccoVeille commented Sep 1, 2024

ghost commented Jul 31, 2024 •

edited by ghost

Loading