tomarrell / wrapcheck

A Go linter to check that errors from external packages are wrapped
https://blog.tomarrell.com/post/introducing_wrapcheck_linter_for_go
MIT License
291 stars 26 forks source link

wrapcheck on goerr113 wrapping %w #35

Closed K4L1Ma closed 1 year ago

K4L1Ma commented 1 year ago

Wrapcheck on goerr113 wrapping %w

I understand that Wrapcheck linter is all about wrapping the error coming from external packages but I think the error coming from the external package should never be used as %w reason being that this forces DevUsers to import the package A where the error is being returned as well as the internal package B where the error is being thrown and possible where the Sentinel lives if we ever need to check on the error type with Error.Is and Error.As creating a form of coupling to the internal detail implementation of package A

import "pkg/Process"

var ErrProccesFailed = "process failed due"
{...}
err := Process()
return fmt.Errorf("%w : %v", ErrProccesFailed,err)

I think Sentinel errors need to be created to wrap the err coming from the external package so only one import is needed to check on the Error.Is or Error.As meanwhile, we make sure the external package error is wrapped into a more meaningful error at this package level without exposing any type of possible coupling

tomarrell commented 1 year ago

Hi @K4L1Ma, thanks for the discussion.

Wrapcheck itself doesn't force the use of %w or %v. Simply that a function matching a configured set of values is called on a returned error. You are free to choose what you want "wrapping" to mean to your project. In the examples, I've used the stdlib fmt.Errorf("%v", err) as a good baseline. I don't think it makes sense to the scope of the project to limit what people can do when they're wrapping errors.

I notice you mention goerr13. I can't control what other linters you use in your project. You will need to decide what works for you. I personally choose not to use goerr13, due to exactly the issue you describe.

As I don't believe this is an issue with wrapcheck, I'll close this issue. Please reopen if you feel there is more to add.

K4L1Ma commented 1 year ago

hi @tomarrell

Actually, my Issue was on Wrapcheck as I think it's exposing the external error forcing a form of coupling if anyone needs to check on the error itself

tomarrell commented 1 year ago

Right, I understand your point of view. However, I don't think it's appropriate for this project to take a stance here and prevent people using a particular wrapping verb.

I have discussed in the documentation what I recommend to prevent coupling.

K4L1Ma commented 1 year ago

in the documentation what I recommend to prevent coupling.

before I go, would you mind pointing me to this?

tomarrell commented 1 year ago

Apologies, not strictly documentation, but I discuss it in this paragraph in the accompanying blog post.

It’s important to keep in mind however, when wrapping with %w, you are implicitly exposing the error to consumers of your package. Only use this when you plan to support the error type and avoid exposing implementation details. You can use the %v verb instead for non-inspectable errors (no errors.Is(), errors.As()).