cedar-go: upgrade golangci-lint checks to v2 and fix all issues found#86
Conversation
| uses: golangci/golangci-lint-action@v4 | ||
| with: | ||
| # Require: The version of golangci-lint to use. | ||
| # When `install-mode` is `binary` (default) the value can be v1.2 or v1.2.3 or `latest` to use the latest version. | ||
| # When `install-mode` is `goinstall` the value can be v1.2.3, `latest`, or the hash of a commit. | ||
| version: v1.56.2 | ||
|
|
||
| # Optional: working directory, useful for monorepos | ||
| # working-directory: somedir | ||
|
|
||
| # Optional: golangci-lint command line arguments. | ||
| # | ||
| # Note: By default, the `.golangci.yml` file should be at the root of the repository. | ||
| # The location of the configuration file can be changed by using `--config=` | ||
| # args: --timeout=30m --config=/my/path/.golangci.yml --issues-exit-code=0 | ||
|
|
||
| # Optional: show only new issues if it's a pull request. The default value is `false`. | ||
| # only-new-issues: true | ||
|
|
||
| # Optional: if set to true, then all caching functionality will be completely disabled, | ||
| # takes precedence over all other caching options. | ||
| # skip-cache: true | ||
|
|
||
| # Optional: if set to true, then the action won't cache or restore ~/go/pkg. | ||
| # skip-pkg-cache: true | ||
|
|
||
| # Optional: if set to true, then the action won't cache or restore ~/.cache/go-build. | ||
| # skip-build-cache: true | ||
|
|
||
| # Optional: The mode to install golangci-lint. It can be 'binary' or 'goinstall'. | ||
| # install-mode: "goinstall" |
There was a problem hiding this comment.
I didn't see any sense in pasting the documentation from the golangci-lint website here.
patjakdev
force-pushed
the
upgrade-golangci-lint
branch
2 times, most recently
from
8309879 to
501e3fe
Compare
| linters: | ||
| disable-all: true | ||
| enable: | ||
| - bodyclose |
There was a problem hiding this comment.
bodyclose doesn't seem relevant for cedar-go
| - govet | ||
| - ineffassign | ||
| - staticcheck | ||
| - unused |
There was a problem hiding this comment.
unused doesn't appear to be a linter any more
| - bodyclose | ||
| - errcheck | ||
| - errname | ||
| - gofmt |
There was a problem hiding this comment.
gofmt is now a formatter...it's not clear to me the difference between gofmt and the default formatter that golangci-lint uses.
corpus_test.go
Outdated
| t.Fatal("error reading corpus compressed archive header", err) | ||
| } | ||
| defer gzipReader.Close() | ||
| defer func() { _ = gzipReader.Close() }() |
There was a problem hiding this comment.
you could also //nolint:errcheck this, which is more likely to survive in the future when someone sees that and is like huh?
internal/eval/evalers_test.go
Outdated
| t.Run("exponentialWithoutCaching", func(t *testing.T) { | ||
| t.Parallel( | ||
| // This test will run for a very long time (O(2^100)) if there isn't caching. | ||
| // This test will run for a very long time (O(2^100)) if there isn't caching. |
There was a problem hiding this comment.
I'm not sure why this comment is in the argument list of a nullary function, and I'm only noticing it because it was reindented. I'd advocate for putting it below t.Parallel() as the t.Parallel() has nothing to do with the comment. Though, even then, putting a comment in the nullary argument list is odd.
There was a problem hiding this comment.
You know, I've seen this happen in the web repo too and had to fix it there. I wonder if there's some code formatter with a bug that's somehow causing this.
apg
left a comment
apg
left a comment
There was a problem hiding this comment.
There are a few comments that I'll defer to you on, as they don't meaningfully change anything. Otherwise, seems good to me.
patjakdev
force-pushed
the
upgrade-golangci-lint
branch
2 times, most recently
from
ee2b411 to
bca33a0
Compare
I preserved the list of linters that we run, but enabled almost all of the revive rules. We can disable them as we see fit in the future. The only regrettable exclusion is on the `exported` rule in the `types/` directory. We should make an effort to document all of the public types. Signed-off-by: Patrick Jakubowski <patrick.jakubowski@strongdm.com>
patjakdev
force-pushed
the
upgrade-golangci-lint
branch
from
bca33a0 to
55a3075
Compare
2 participants
Issue #, if available:
Description of changes: I ran
golangci-lint run ./... --fixand it made all of these changes. At a cursory glance, they all seem to make sense.I changed the config to enable the same set of linters but turned on almost all the
reviverules. I disabledexportedfor theinternalandxdirectories, which seems legit to me, but also had to disable it for thetypesdirectory for now until we can get our documentation up to snuff.