-
Notifications
You must be signed in to change notification settings - Fork 55
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
RFE: misc nits #73
RFE: misc nits #73
Conversation
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.
Looks good to me.
Acked-by: Paul Moore <paul@paul-moore.com>
All the files in the package (except the recently added seccomp_ver_test.go) have linux build tag. Since seccomp is linux-specific, it does not make much sense to add this build tag, because this is the only GOOS supported, and it won't compile on any other platform anyway. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 8b6c34e adds go.sum with checksums of some seemingly random packages. Since libseccomp-golang do not import (use) any of those packages, these checksums are not required (and are probably leftovers of running "go get" after cd to this directory). This commit is a result of "go mod tidy" run. Fixes: 8b6c34e Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a result of running "gofumpt -w *.go" on the source code. gofumpt[1] is a tool similar to (and based on) gofmt, which adds some some extra rules for better code formatting. Here, it removes some extra vertical whitespace, and replaces a var() block with a single element inside with a mere var declaration. Both make sense. Enable gofumpt in golangci-lint config to make sure future commits are gofumpt-ed. [1] https://github.com/mvdan/gofumpt Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
First commit removed (will be part of #70 instead), so this becomes even more trivial to review. @drakenclimber PTAL |
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.
Looks good to me. I'll merge it in a bit.
Acked-by: Tom Hromatka <tom.hromatka@oracle.com>
One question, @kolyshkin - I'm looking through the Github Actions log right now. Where does Github Actions run the gofumpt linter? I don't see it in any of the logs |
It's part of golangci-lint run, which does not print the names of enabled linters (unless those report issues). So what you can see in the logs is
It's similar locally: kir@kd:~/go/src/github.com/seccomp/libseccomp-golang$ make lint
golangci-lint run .
kir@kd:~/go/src/github.com/seccomp/libseccomp-golang$ Although you can see which linters are enabled by running
Now, golangci-lint would error out if a linter is specified which can't be run: kir@kd:~/go/src/github.com/seccomp/libseccomp-golang$ cat .golangci.yml
# For documentation, see https://golangci-lint.run/usage/configuration/
linters:
enable:
- gofumpt
kir@kd:~/go/src/github.com/seccomp/libseccomp-golang$ sed -i 's/gofumpt/gofoo/' .golangci.yml
kir@kd:~/go/src/github.com/seccomp/libseccomp-golang$ golangci-lint run .
ERRO Running error: unknown linters: 'gofoo', run 'golangci-lint help linters' to see the list of supported linters
kir@kd:~/go/src/github.com/seccomp/libseccomp-golang$ Ultimately, though, if you need to be 100% sure, open a test DNM/draft PR with a deliberately introduced issue which gofumpt is supposed to fix. |
These are the collection of small source code cleanups and nits. No functional change. See individual commits for details.