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

IsAny() multi error support #140

Merged
merged 1 commit into from
May 24, 2024

Conversation

nikitacrit
Copy link
Contributor

@nikitacrit nikitacrit commented May 6, 2024

Hello! At the moment the following code will produce the result "no". I suggest a fix based on the code found in the original method Is().

func main() {
	err := errors.Join(io.EOF, errors.New("gopher"))

	if errors.IsAny(err, io.EOF, net.ErrClosed) {
		fmt.Print("yes")
	} else {
		fmt.Print("no")
	}
}

This change is Reviewable

@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented May 6, 2024

CLA assistant check
All committers have signed the CLA.

@nikitacrit
Copy link
Contributor Author

@dhartunian Hi David. I see your latest activity in this repository and would like to ask if contributions are welcome and if someone can review the changes?

@dhartunian dhartunian self-requested a review May 7, 2024 18:02
@dhartunian
Copy link
Contributor

@nikitacrit thanks for the ping, I'll take a look when I get a chance, probably over the next few days.

@dhartunian dhartunian self-assigned this May 7, 2024
"github.com/cockroachdb/redact"
"github.com/gogo/protobuf/proto"

"github.com/cockroachdb/errors/errbase"
"github.com/cockroachdb/errors/errorspb"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: revert the import reordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! And sorry I was forced to force push to keep the commit history clean.

@@ -361,6 +362,9 @@ func TestIsAny(t *testing.T) {
err2 := errors.New("world")
err3 := pkgErr.Wrap(err1, "world")
err4 := pkgErr.Wrap(err2, "universe")
err5 := errors.Join(err1, errors.New("gopher"))
err6 := errors.Join(errors.New("gopher"), err2)
err7 := errors.Join(err1, err2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test case that wraps at the top level? That will ensure that the implementation is necessary within the UnwrapOnce loop instead of outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@nikitacrit nikitacrit force-pushed the IsAny-multi-error-support branch from 51ebea5 to b0e6c50 Compare May 15, 2024 20:27
@nikitacrit nikitacrit requested a review from dhartunian May 15, 2024 20:31
Copy link
Contributor

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm: thanks for the changes.

Reviewed 1 of 2 files at r2.
Reviewable status: 1 of 2 files reviewed, all discussions resolved


markers/markers.go line 26 at r1 (raw file):

Previously, nikitacrit (Nikita Sidorov) wrote…

Done! And sorry I was forced to force push to keep the commit history clean.

👍 I prefer squashed commits

@dhartunian dhartunian merged commit f0b6870 into cockroachdb:master May 24, 2024
6 of 7 checks passed
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