search

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

Linter triggers on wrapper functions #11

Closed invidian closed 3 years ago

invidian commented 3 years ago

Considering following code:

// Deploy checks current status of deployed group of instances and updates them if there is some
// configuration drift.
func (a *apiLoadBalancers) Deploy() error {
  return a.containers.Deploy()
}

Right now linter triggers on it, but it seems very counter-intuitive.

Used via golangci-lint version v1.39.0

tomarrell commented 3 years ago

Would you be able to provide a little more info into the type of the a.containers value. Is it an interface by any chance?

invidian commented 3 years ago

Would you be able to provide a little more info into the type of the a.containers value. Is it an interface by any chance?

Yes, a.containers is an container management interface. apiLoadBalancers type exposes similar functionality but of more specific type.

tomarrell commented 3 years ago

I see. So this is expected functionality if the method is being called through an interface as the implementation may come from an arbitrary package. In this case here, I would suggest it may make sense to wrap the error being returned from this call as per the criteria:

  1. Errors returned from another package.
  2. Errors returned by interface methods. Interfaces may be implemented by a separate package, therefore wrapping the errors here with context may be valuable.

I understand that this may be a little inconvenient as this previously wasn't reported (due to direct method calls not being considered). But in a future release I will add the ability for more fine-grained configuration.