Fix various linting issues and minor bugs #280
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thaJeztah
commented
Aug 22, 2024
assert/cmp/compare.go
Outdated
| //nolint:errorlint // unwrapping is not appropriate here | ||
| // unwrapping is not appropriate here |
There was a problem hiding this comment.
LOL looks like different versions of the linters disagree. Let me add nolintlint to the ignore list;
assert/cmp/compare.go:252:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
if _, ok := err.(causer); ok {
^
thaJeztah
force-pushed
the
linting_errors
branch
from
65d6780 to
6d6c58c
Compare
Current versions of this linter default to no allowing third-party
dependencies by default (only stdlib and golang.org/x/sys).
As there's currently no configuration set to prevent a specific import,
this results in some imports being flagged;
fs/manifest_test.go:11:2: import 'github.com/google/go-cmp/cmp' is not allowed from list 'Main' (depguard)
"github.com/google/go-cmp/cmp"
^
assert/assert.go:96:2: import 'github.com/google/go-cmp/cmp' is not allowed from list 'Main' (depguard)
gocmp "github.com/google/go-cmp/cmp"
^
assert/assert_test.go:8:2: import 'github.com/google/go-cmp/cmp' is not allowed from list 'Main' (depguard)
gocmp "github.com/google/go-cmp/cmp"
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
The nolintlint linter considers the `nolint` to be redundant;
assert/cmp/compare.go:251:2: directive `//nolint:errorlint // unwrapping is not appropriate here` is unused for linter "errorlint" (nolintlint)
//nolint:errorlint // unwrapping is not appropriate here
^
But depending on the go version, it may trigger a warning:
assert/cmp/compare.go:252:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
if _, ok := err.(causer); ok {
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
assert/assert_test.go:108:3: empty-block: this block is empty, you can remove it (revive)
for range []int{1, 2, 3, 4} {
}
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
linting_errors
branch
from
6d6c58c to
d65bc1e
Compare
ccoVeille
reviewed
Aug 28, 2024
internal/source/source.go:75:2: return both the `nil` error and invalid value: use a sentinel error instead (nilnil)
return nil, nil
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
"cap" is now a builtin:
internal/source/source_test.go:50:2: redefines-builtin-id: redefinition of the built-in function cap (revive)
cap := &capture{}
^
internal/source/source_test.go:73:2: redefines-builtin-id: redefinition of the built-in function cap (revive)
cap := &capture{}
^
internal/source/source_test.go:86:2: redefines-builtin-id: redefinition of the built-in function cap (revive)
cap := &capture{}
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
fs/report_test.go:187:19: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive)
matcher := func(b []byte) CompareResult {
^
fs/report_test.go:196:19: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive)
matcher := func(b []byte) CompareResult {
^
fs/report.go:107:46: printf: non-constant format string in call to gotest.tools/v3/fs.existenceProblem (govet)
p = append(p, existenceProblem("content", r.FailureMessage()))
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
assert/assert_test.go:249:12: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive)
f := func(b bool) {}
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Current versions of govet check for printf functions called with a format
string, but no arguments. This results in linting errors:
fs/report.go:107:46: printf: non-constant format string in call to gotest.tools/v3/fs.existenceProblem (govet)
p = append(p, existenceProblem("content", r.FailureMessage()))
^
This patch changes existenceProblem to use format.Message, which is used
elsewhere for similar situations.
Signed-off-by: Sebastiaan van Stijn <[email protected]>
poll/example_test.go:38:16: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
check := func(t poll.LogT) poll.Result {
^
poll/poll_test.go:21:21: unused-parameter: parameter 'args' seems to be unused, consider removing or renaming it as _ (revive)
func (t *fakeT) Log(args ...interface{}) {}
^
poll/poll_test.go:23:22: unused-parameter: parameter 'format' seems to be unused, consider removing or renaming it as _ (revive)
func (t *fakeT) Logf(format string, args ...interface{}) {}
^
poll/poll_test.go:28:16: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
check := func(t LogT) Result {
^
poll/poll.go:154:18: printf: non-constant format string in call to gotest.tools/v3/poll.Continue (govet)
return Continue(buf.String())
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
assert/opt/opt_test.go:254:20: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func TestPathDebug(t *testing.T) {
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
x/subtest/example_test.go:47:23: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func startFakeService(t subtest.TestContext) string {
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
poll/poll.go:154:18: printf: non-constant format string in call to gotest.tools/v3/poll.Continue (govet)
return Continue(buf.String())
^
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
linting_errors
branch
from
d65bc1e to
d21c522
Compare
ccoVeille
reviewed
Aug 28, 2024
Comment on lines
20
to
+23
|
|
||
| func (t *fakeT) Log(args ...interface{}) {} | ||
| func (t *fakeT) Log(...interface{}) {} | ||
|
|
||
| func (t *fakeT) Logf(format string, args ...interface{}) {} | ||
| func (t *fakeT) Logf(string, ...interface{}) {} |
There was a problem hiding this comment.
Maybe you coule use any instead of interface{}
There was a problem hiding this comment.
I kept interface{} for now, so that we didn't have to raise the go version in go.mod;
Line 3 in 631f6c8
(any would require go1.18 as minimum; https://go.dev/ref/spec#Go_1.18)
There was a problem hiding this comment.
I know 😬 but Go 1.18 is now not only old, but no longer supported
There was a problem hiding this comment.
Yup, more than aware; I also know that it's not uncommon for companies to maintain an (internal) LTS version of older Go versions. Not all of those will be visible publicly on GitHub. (e.g. I know Google for a long time required compatibility with go1.13 for some of their systems - they maintained their own version for that).
| @@ -50,8 +50,8 @@ func errProblem(reason string, err error) problem { | |||
| return problem(fmt.Sprintf("%s: %s", reason, err)) | |||
| } | |||
|
|
|||
| func existenceProblem(filename, reason string, args ...interface{}) problem { | |||
| return problem(filename + ": " + fmt.Sprintf(reason, args...)) | |||
| func existenceProblem(filename string, msgAndArgs ...interface{}) problem { | |||
There was a problem hiding this comment.
Maybe you coule use ...any instead of ...interface{}
dnephin
approved these changes
Aug 29, 2024
Yes, if there's a need to bump, that's probably fine. I tried to avoid it if there's no strict / important reason, knowing that gotest.tools is a test-dependency, and (library) projects may want to continue support for old Go versions, therefore running tests against those, so it's worth trying to keep the requirements as low as reasonably possible. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See individual commits for details.