Enable all linters and resolve all outstanding lint issues (Resolves issue 2386) #2455

James9074 · 2025-06-17T14:49:33Z

As per #2386, this PR uncomments gocritic, govet, and testifylint, and resolves all outstanding lint findings. Note that govet did not actually return anything, but gocritic and testifylint did. Those were resolved, along with a number of stubborn yaml comment spacing complaints within the github workflows yamls.

I generally resolved each finding in its own commit, for clarity. Additionally, I ran the full suite of unit tests after each fix and ensured no bloat in test execution time. The test suite took ~2.5 minutes before and after all changes on my machine.

Before:

See this for the raw output of all findings as of main/HEAD

After:

mage lint:all                                                                                                                                                    ✔  at 04:25:57 PM  
generating buf
generating go
Generating options for common.SchemaInformation...
Generated 1 options
Generating options for graph.ConcurrencyLimits...
Generated 1 options
Generating options for options.ExperimentalServerOptions...
Generated 1 options
Generating options for datastore.ConnPoolConfig...
Generated 1 options
Generating options for datastore.Config...
Generated 1 options
Generating options for datastore.RelIntegrityKey...
Generated 1 options
Generating options for server.CacheConfig...
Generated 1 options
Generating options for server.MiddlewareOption...
Generated 1 options
Generating options for server.Config...
Generated 1 options
Generating options for testserver.Config...
Generated 1 options
Generating options for util.GRPCServerConfig, HTTPServerConfig...
Generated 1 options
Generating options for options.QueryOptions, ReverseQueryOptions, RWTOptions...
Generated 1 options
Generating options for options.DeleteOptions...
Generated 1 options
running vulncheck
formatting go
running golangci-lint
running analyzers
0 issues.

…Regex, use regexp.MustCompile

…st function (testifylint) - replace with manual error checking

…alse()

github-actions · 2025-06-17T14:50:16Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

James9074 · 2025-06-17T14:51:07Z

I have read the CLA Document and I hereby sign the CLA

… it does not degrade test quality

…st function (testifylint) - Move require.Error outside of goroutine

…nged to InEpsilon for approx comparison, resulting in more stable test results

…h `validationString += "[...]"` (gocritic)

…tic)

tstirrat15

This looks good to me!

James9074 · 2025-06-19T16:49:24Z

@tstirrat15 Anything else I need to get this merged before it gets stale? Unable to merge it myself - not sure if @vroldanbet has to review or if he was auto-included.

vroldanbet

Thanks for your contribution! ✨ I understand it can get stale easily, but please be patient while we review this low-priority PR; it's a bit lengthy.

internal/datastore/crdb/migrations/zz_migration.0008_add_transaction_metadata_table.go

internal/datastore/crdb/migrations/zz_migration.0009_add_expiration_support.go

internal/datastore/postgres/xid8.go

internal/datastore/revisions/hlcrevision_test.go

internal/caveats/builder_test.go

internal/dispatch/singleflight/singleflight_test.go

internal/middleware/perfinsights/perfinsights_test.go

pkg/schemadsl/compiler/translator.go

.golangci.yaml

tstirrat15 · 2025-07-07T17:24:51Z

@James9074 just wanted to update - this is still something we're interested in and we haven't had the bandwidth to look at it yet. Apologies for the delay.

tstirrat15 · 2025-08-26T16:15:39Z

@James9074 we're finally done with the push and should have some more bandwidth now. do you have the time and energy to get this rebased and ready? If not, we can take over the PR and make sure that you're attributed.

James9074 · 2025-08-26T21:58:40Z

@James9074 we're finally done with the push and should have some more bandwidth now. do you have the time and energy to get this rebased and ready? If not, we can take over the PR and make sure that you're attributed.

Awesome! No worries, I'll rebase this week and resubmit for review.

vroldanbet

Thanks with your patience with this one, we appreciate it ✨
I finished reviewing all the files. There are a handful of things I'd like to clarify before we move forward.

Please address the conflicts and I'll review again.

internal/datastore/common/gc_test.go

pkg/schema/typesystem_test.go

pkg/releases/releases_test.go

pkg/development/assertions.go

pkg/datastore/test/revisions.go

vroldanbet · 2025-08-27T11:14:07Z

internal/datastore/proxy/schemacaching/watchingcache_test.go

+		var nsNotFoundErr datastore.NamespaceNotFoundError
+		if err == nil || !errors.As(err, &nsNotFoundErr) {
+			t.Errorf("expected NamespaceNotFoundError, got: %v", err)
+		}
+		if wcache.namespaceCache.inFallbackMode {
+			t.Errorf("expected inFallbackMode to be false")
+		}


What happened here? require ultimately causes t to fail. And why does not abide by the same recommendations described here?

same for all other instances in this file

vroldanbet · 2025-08-27T11:25:45Z

internal/testfixtures/datastore.go


 		if index > len(expected)-1 {
-			tc.Require.Fail("expected %s, but found no additional results", expectedStr)
+			tc.Require.Fail("expected more results, but found no additional results", expectedStr)


Could you tell me what you're trying to fix here? How is expected more results, but found no additional results a better error message?

pkg/datastore/test/counters.go

vroldanbet · 2025-08-27T11:35:57Z

pkg/composableschemadsl/compiler/translator.go

-		leftOps := collapseOps(leftOperation, lookup)
-		rightOps := collapseOps(rightOperation, lookup)
-		ops := append(leftOps, rightOps...)
+		ops := append(collapseOps(leftOperation, lookup), collapseOps(rightOperation, lookup)...)


Could you tell me the reason for this change?

pkg/cmd/util/util.go

James9074 · 2025-10-09T13:57:50Z

@tstirrat15 Feel free to take this one over or close it out, unfortunately I'm short on time this quarter and don't think I'll be able to wrap this up before it's too stale.

tstirrat15 · 2025-10-09T22:55:04Z

Sounds good. Thank you so much for the work you put into it!

miparnisari · 2025-10-21T17:58:34Z

Thank you @James9074! Your hard work doesn't go unnoticed. We'll take this over the finish line :)

James9074 added 12 commits June 16, 2025 23:15

Enable all linters and resolve yaml lint issues

8f5eb3a

Yaml comment lint fixes

b3bf8ff

Capitalizing NewXid8 to properly export it for use in tests

0e4538a

codecov.yaml should start with ---

6ac1b7d

Resolving gocritic: regexpMust: for const patterns like semver.SemVer…

750111c

…Regex, use regexp.MustCompile

Resolving gocritic: ifElseChain: rewrite if-else to switch statement

5191115

Automatic lint changes: require.* simplifications and unused imports

729b7e7

ifElseChain: rewrite if-else to switch statement (gocritic)

2084c41

assignOp: replace = with += (gocritic)

0dad75a

watch.go - ifElseChain: rewrite if-else to switch statement (gocritic)

37bd9e4

go-require: require must only be used in the goroutine running the te…

36a58d3

…st function (testifylint) - replace with manual error checking

useless-assert: meaningless assertion (testifylint) - Fixed require.F…

d565671

…alse()

James9074 mentioned this pull request Jun 17, 2025

Enable go vet's linters #2386

Open

authzedbot added a commit to authzed/cla that referenced this pull request Jun 17, 2025

@James9074 has signed the CLA in authzed/spicedb#2455

7a0b311

James9074 added 7 commits June 17, 2025 11:11

Opting to ignore line on require.ErrorAs to reduce test code bloat as…

6524f1a

… it does not degrade test quality

go-require: require must only be used in the goroutine running the te…

fbed941

…st function (testifylint) - Move require.Error outside of goroutine

float-compare: use require.InEpsilon (or InDelta) (testifylint) > Cha…

b21d7f7

…nged to InEpsilon for approx comparison, resulting in more stable test results

assignOp: replace validationString = validationString + "[...]" wit…

e881842

…h `validationString += "[...]"` (gocritic)

elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocri…

c42c64a

…tic)

ifElseChain: rewrite if-else to switch statement (gocritic)

06f3cc5

badCond: condition is suspicious (gocritic)

a0e60bc

tstirrat15 previously approved these changes Jun 18, 2025

View reviewed changes

Merge branch 'main' into resolve-lint-issues

b1cafbd

vroldanbet requested changes Jun 19, 2025

View reviewed changes

Reverting a few changes based on @vroldanbet's feedback

d170139

James9074 dismissed tstirrat15’s stale review via d170139 June 19, 2025 18:14

Merge branch 'main' into resolve-lint-issues

b8e8e42

James9074 requested a review from vroldanbet June 19, 2025 18:21

Meyazhagan reviewed Jun 24, 2025

View reviewed changes

.golangci.yaml Show resolved Hide resolved

James9074 added 3 commits June 24, 2025 16:27

Merge branch 'main' into resolve-lint-issues

551663c

Merging upstream

341b0ec

Merge branch 'main' into resolve-lint-issues

b1ee733

vroldanbet requested changes Aug 27, 2025

View reviewed changes

This was referenced Oct 22, 2025

chore: Lint fixes part 6 #2635

Merged

chore: Lint fixes part 7 #2637

Merged

Lint fixes part 8 #2644

Merged

Refactor GC and GC tests to be more reliable #2645

Draft

Lint fixes part 10 #2655

Merged

This was referenced Oct 29, 2025

Lint fixes part 11 #2667

Merged

Lint fixes part 12 #2668

Merged

Enable all linters and resolve all outstanding lint issues (Resolves issue 2386) #2455

Are you sure you want to change the base?

Enable all linters and resolve all outstanding lint issues (Resolves issue 2386) #2455

Uh oh!

Conversation

James9074 commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

James9074 commented Jun 17, 2025

Uh oh!

tstirrat15 left a comment

Choose a reason for hiding this comment

Uh oh!

James9074 commented Jun 19, 2025

Uh oh!

vroldanbet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tstirrat15 commented Jul 7, 2025

Uh oh!

tstirrat15 commented Aug 26, 2025

Uh oh!

James9074 commented Aug 26, 2025

Uh oh!

vroldanbet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vroldanbet Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vroldanbet Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vroldanbet Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

James9074 commented Oct 9, 2025

Uh oh!

tstirrat15 commented Oct 9, 2025

Uh oh!

miparnisari commented Oct 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

James9074 commented Jun 17, 2025 •

edited

Loading

github-actions bot commented Jun 17, 2025 •

edited

Loading

vroldanbet Aug 27, 2025 •

edited

Loading