Enabling the errcheck linter about weaver HOT 2 CLOSED

Hi Daniel,

We would love to take you up on your offer to help enable errcheck.

Your suggestion on just focusing on non-test code for now seems spot on.

_, _ = w.Write(...)
// or
w.Write(...) //nolint:errcheck

I think I would prefer the latter since it will be easier to spot and search for. So let's go with the //nolint:errcheck approach.

Thanks for offering to help and your clear write-up of the types of changes that would be needed.

from weaver.

Initially I'd want to enable it only for production code (not tests), to limit the number of changes required to get it enabled. Something like this:

issues:
  exclude-rules:
    - path: '_test\.go'
      linters: [errcheck]

I think a bunch of the unhandled errors are safe to ignore (ex: writing an HTTP response from a handler). Those can be ignored with either:

_, _ = w.Write(...)
// or
w.Write(...) //nolint:errcheck

Do you have a preference on one of those styles?

There are a few cases where it seems like the error simply needs to be returned:

or logged:

A log here to inform the user that the port is in use would probably help avoid confusion when perfetto isn't able to display the trace.

The remaining cases of unhandled errors seems to be mostly in these two categories:

1. starting goroutines with a function that returns an error - in a few of these cases the only possible return value is ctx.Err. I wonder if the error return value could be removed from the function signature in those cases.
2. sending an RPC response from an RPC handler when the operation is performed in a goroutine. In these cases I expect the error is also safe to ignore because the client receives the error response, and the caller doesn't care about it.

from weaver.