Fix various linting issues and minor bugs#280
Merged
dnephin merged 12 commits intogotestyourself:mainfrom Aug 29, 2024
Merged
Conversation
thaJeztah
commented
Aug 22, 2024
assert/cmp/compare.go
Outdated
|
|
||
| func formatErrorMessage(err error) string { | ||
| //nolint:errorlint // unwrapping is not appropriate here | ||
| // unwrapping is not appropriate here |
Contributor
Author
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{}
Contributor
Author
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
Contributor
Author
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).
|
|
||
| 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
Member
dnephin
left a comment
dnephin
left a comment
There was a problem hiding this comment.
Thank you!
We should update the Go minimum version sometime soon.
Contributor
Author
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. |
Crown0815
pushed a commit
to Crown0815/forgejo-runner-windows
that referenced
this pull request
May 21, 2025
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [gotest.tools/v3](https://github.com/gotestyourself/gotest.tools) | require | patch | `v3.5.1` -> `v3.5.2` | --- ### Release Notes <details> <summary>gotestyourself/gotest.tools (gotest.tools/v3)</summary> ### [`v3.5.2`](https://github.com/gotestyourself/gotest.tools/releases/tag/v3.5.2) [Compare Source](gotestyourself/gotest.tools@v3.5.1...v3.5.2) #### What's Changed - assert: ensure message is always displayed & fix under bazel by [@​cstrahan](https://github.com/cstrahan) in gotestyourself/gotest.tools#276 - go.mod: golang.org/x/tools v0.13.0 for go1.22+ compatibility by [@​thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#282 - poll: Continue(): use format.Message for formatting by [@​thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#279 - fix TestFromDirSymlink on Windows due to missing drive-letter by [@​thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#283 - Fix various linting issues and minor bugs by [@​thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#280 - fix badges in readme, gofmt, and minor linting fix by [@​thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#284 - circleci: add go1.21, go1.22, go1.23, and update golangci-lint to v1.60.3 by [@​thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#285 - assert, assert/cmp: un-deprecate assert.ErrorType for now by [@​thaJeztah](https://github.com/thaJeztah) in gotestyourself/gotest.tools#286 #### New Contributors - [@​cstrahan](https://github.com/cstrahan) made their first contribution in gotestyourself/gotest.tools#276 **Full Changelog**: gotestyourself/gotest.tools@v3.5.1...v3.5.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC), Automerge - Between 12:00 AM and 03:59 AM ( * 0-3 * * * ) (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4yMTIuMCIsInVwZGF0ZWRJblZlciI6IjM5LjI1Mi4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/524 Co-authored-by: Renovate Bot <[email protected]> Co-committed-by: Renovate Bot <[email protected]>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.