-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci(lint): enable errcheck, errorlint, gci, misspell, nonamedreturns and staticcheck linters #71
Conversation
Signed-off-by: Matthieu MOREL <[email protected]>
Signed-off-by: Matthieu MOREL <[email protected]>
Signed-off-by: Matthieu MOREL <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for this change.
Had one qq... also, be great to add some really useful ones like revive
, although that should probably land in a later PR to keep the scope of this one straightforward.
f089a27
to
e986c62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 overall, a few small comments
netns_others.go
Outdated
return -1, ErrNotImplemented | ||
} | ||
|
||
func NewNamed(name string) (NsHandle, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I prefer the _
convention, however sometimes for primitive types, keeping the arg name is helpful... for example here I'd rather keep this one because even though unused, it clarifies to the caller what the arg is intended for. That way if we do ever implement it, callers are already ready for it.
Same with all the rest of the following args in this file...
Can you please add a per-line exception?
nshandle_linux.go
Outdated
// associated with the network handle. | ||
func (ns NsHandle) UniqueId() string { | ||
func (ns NsHandle) UniqueID() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change is it not?
In general I don't mind breaking changes like this where the type checker will complain, but we'd need to ensure to bump the major version.
And also, I do wonder, is it really worth it to make a breaking change purely for a cosmetic lint?
Signed-off-by: Matthieu MOREL <[email protected]> up Signed-off-by: Matthieu MOREL <[email protected]>
Signed-off-by: Matthieu MOREL <[email protected]>
Your two comments are against revive's rules. |
Yes, I'm aware, that's why I mentioned adding specific exceptions for violations.
Sounds fine. Thanks for contributing this PR! |
Signed-off-by: Matthieu MOREL [email protected]