Improve global error detection #32
Conversation
Only treat global error variables as such if they have an "err" or "Err" prefix and implement the error interface. Implements leighmcculloch#5.
leonklingele
force-pushed
the
improve-error-detection
branch
from
d5bf92f to
0eb5c38
Compare
|
I would like to suggest making that prefix configurable, i would like to use the error interface implementation feature, but not that err or Err prefixes |
|
This looks good to me, but I'm a little concerned with the fact that this is a breaking change potentially as it reduces the surface area of what is allowed. It's okay for us to increase the surface area of what is allowed but reducing it may break existing builds. Maybe that's okay. Or maybe this gets released in a new major version. Does anyone else have thoughts on this? |
|
@hurrycaner the idea to use the err prefix for non error globals sounds out of scope of this tool. |
|
Hey @leighmcculloch, can this be merged? 😊 |
checknoglobals/check_no_globals.go
Outdated
| prefix := "err" | ||
| if i.IsExported() { | ||
| prefix = "Err" | ||
| } | ||
| return strings.HasPrefix(i.Name, prefix) | ||
|
|
||
| hasErrPrefix := strings.HasPrefix(i.Name, prefix) | ||
| if !hasErrPrefix { | ||
| return false | ||
| } |
There was a problem hiding this comment.
@leonklingele @bombsimon Do you folks have thoughts on if we should remove the err/Err prefix requirement? Regardless of the name given to the global variable, just checking if it is an error seems reasonable to me.
There was a problem hiding this comment.
I'd leave it as-is. We could make it configurable with a default that still enforces the prefix but I don't really see a benefit in doing so.
|
Release in v0.2.0. |
Only treat global error variables as such if they have an "err" or "Err" prefix
and implement the error interface.
Close #5