Conversation
Commented out incompatible options in the pre-existing `.golangci.yml` and ran `golangci-lint migrate`. Resources: - https://golangci-lint.run/docs/product/migration-guide/
The `make` command was failing at the golangci-lint install step due to a checksum verification error. Switched to the new install url. Resources: - golangci/golangci-lint#6745
The v1 -> v2 config migration expanded the old presets into a huge explicit enable list. Slim it back down to `default: standard` plus the linters we actually configure, keep the old disable list as a guard, and silence the staticcheck quickfix that wants to rewrite upstream's `a.tt.` selector style.
|
One small note: the linter |
kinbiko
left a comment
There was a problem hiding this comment.
I've reviewed this code before reviewing the other PR's most recent changes. There's a good chance I'll merge the other PR (don't know yet). If I do, we can discuss other things you can contribute.
| simplify: true | ||
| goimports: | ||
| local-prefixes: | ||
| - github.com/kinbiko/bugsnag |
There was a problem hiding this comment.
I think this is leftover copy+pasta, and can be removed.
|
|
||
| version: "2" | ||
| linters: | ||
| # `standard` enables errcheck, govet, ineffassign, staticcheck, unused: https://golangci-lint.run/docs/linters/#all-linters |
There was a problem hiding this comment.
I do actually like the idea of having new linters being introduced as the language and best practices evolve causing build failures, instead of having to manually keep track of linters being introduced in golangci-lint.
As an exercise there's value in learning about the judgments in the linter violations that'll (presumably) appear if we keep the configs/disabled list the same (up to renames).
| - linters: | ||
| - dupl | ||
| - errcheck | ||
| - errchkjson | ||
| - forbidigo | ||
| - funlen | ||
| - gocognit | ||
| - gocyclo | ||
| - lll | ||
| - mnd | ||
| - staticcheck | ||
| - testpackage | ||
| path: _test\.go | ||
| paths: | ||
| - third_party$ | ||
| - builtin$ | ||
| - examples$ |
There was a problem hiding this comment.
Can simplify here I think.
| - linters: | |
| - dupl | |
| - errcheck | |
| - errchkjson | |
| - forbidigo | |
| - funlen | |
| - gocognit | |
| - gocyclo | |
| - lll | |
| - mnd | |
| - staticcheck | |
| - testpackage | |
| path: _test\.go | |
| paths: | |
| - third_party$ | |
| - builtin$ | |
| - examples$ | |
| - linters: | |
| - gocognit | |
| - funlen | |
| - lll | |
| path: _test\.go |
| range-loops: true | ||
| for-loops: false | ||
| staticcheck: | ||
| # QF1008 keeps suggesting dropping the `tt.` prefix; to preserve the original Asserter API we're choosing to ignore this warning. |
There was a problem hiding this comment.
Nah, I think it should be included. I do expect changes to .go files as part of this change, as the code is updated to adopt to the new language idioms.
| paths: | ||
| - third_party$ | ||
| - builtin$ | ||
| - examples$ |
| @@ -1,4 +1,4 @@ | |||
| LINTER_VERSION := v1.61.0 | |||
| LINTER_VERSION := v2.13.2 | |||
There was a problem hiding this comment.
This solutions is alright, but since 1.24, Go has added the idea of a dev-dependency.
I'd like to use this to maintain the golangci-lint version in a more idiomatic way, that tools like dependabot understand.
A requirement that's likely going to make the naive and obvious solution insufficient: The github.com/kinbiko/jsonassert module MUST have 0 transitive dependencies, i.e. any users of this library should not implicitly download linter-related code.
Hi @kinbiko,
I saw that a PR for this already exists (#50), but for learning purposes I still wanted to give it a try myself 😄
This PR bumps Go to 1.27 and golangci-lint to v2.13.2, and updates the GitHub Actions build job.
Details
Makefile install fix for golangci-lint. While running
make lint, the install step failed with a checksum error. The old install script identifies the release file with a loose filename match, so the extra.sbom.jsonfiles that newer releases ship break the comparison. Pointing the URL to the recommended install script fixes this, with no other changes to the Makefile.Reference: #6539. The v2.12.1 changelog also documents this:
Lint config. The v1 to v2 config migration expanded the old
presetskey into a long explicit list of enabled linters, so we ended up with many linters enabled that were never chosen individually:Since
presetsno longer exists in v2, this PR uses the defaultstandardset and keeps only the linters that we explicitly configure. The olddisable:list is kept as a safeguard, and two lint warnings are suppressed on purpose:goconstignores test files, because the repeated JSON strings in the table tests are intentional test fixturesa.tt.Errorf(...)asa.Errorf(...), and we prefer to keep the style the codebase already usesNote
Everything else in the config and in the library itself is unchanged, and
make checkpasses.Closes #49
Checklist