Skip to content
This repository has been archived by the owner on Mar 30, 2022. It is now read-only.

Wrap errors #26

Merged
merged 1 commit into from
Dec 29, 2021
Merged

Wrap errors #26

merged 1 commit into from
Dec 29, 2021

Conversation

dshemin
Copy link

@dshemin dshemin commented Dec 28, 2021

I think will be better to use go 1.13 errors wrapping.

@hnakamur
Copy link
Owner

Hi, thanks for your pull request.

However I think wrapping an error should be used where it is absolutely necessary to handle the error by checking the internal error type or details. I became to think so after reading the following.

Working with Errors in Go 1.13 - The Go Programming Language

In other words, wrapping an error makes that error part of your API. If you don’t want to commit to supporting that error as part of your API in the future, you shouldn’t wrap the error.

In this library, I think there is no such place since it does not return public error types.
So I think it is better not to wrap errors.

@dshemin
Copy link
Author

dshemin commented Dec 29, 2021

Yes, it's true, you should do something when it should be done. But this is a library and you can't predict how it will be used. For instance, I use it in my project and I have special cases for context.Canceled and context.DeadlineExceeded and I don't want to use string comparison for errors in my production code. I prefer to use errors.Is and errors.As, for instance like this if errors.Is(err, context.Canceled) { ... }. So I had to write next code, to adopt errors from go-scp package:

func adoptError(err error) error {
	if err == nil {
		return nil
	}

	switch {
	case strings.Contains(err.Error(), context.Canceled.Error()):
		err = wrapError{
			msg: err.Error(),
			err: err,
		}
	case strings.Contains(err.Error(), context.DeadlineExceeded.Error()):
		err = wrapError{
			msg: err.Error(),
			err: err,
		}
	}
	return err
}

type wrapError struct {
	msg string
	err error
}

func (e wrapError) Error() string { return e.msg }
func (e wrapError) Unwrap() error { return e.err }

@hnakamur
Copy link
Owner

Thanks for your explanation.
I'm convinced now, so I merge this.
Thanks!

@hnakamur hnakamur merged commit 8faebab into hnakamur:main Dec 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants