chore: pull in lint rules from spicedb #600
Conversation
tstirrat15
force-pushed
the
enable-more-linters
branch
from
40c6386 to
ac043d1
Compare
| if backupAlreadyExisted { | ||
| if os.IsNotExist(err) || len(readCursor) == 0 { | ||
| return nil, nil, fmt.Errorf("backup file %s already exists", backupFileName) |
There was a problem hiding this comment.
This was a bit of renesting in favor of implementing the same logic with a case statement - we were previously checking if backupAlreadyExisted && <something> and then && <somethingElse>.
| require.NoError(srv.Run(ctx)) | ||
| assert.NoError(t, srv.Run(ctx)) |
There was a problem hiding this comment.
This is working around the "no require in goroutines" issue
internal/cmd/schema_test.go
Outdated
| } else { | ||
| require.Error(err) | ||
| require.ErrorAs(err, &tc.expectErr) | ||
| require.ErrorIs(err, tc.expectErr) |
There was a problem hiding this comment.
Gotta make sure this assertion is correct - as previously written it always passed.
| for _, example := range examples { | ||
| instructions += "<example>" + string(example) + "</example>\n" | ||
| instructions.WriteString("<example>") | ||
| instructions.Write(example) | ||
| instructions.WriteString("</example>\n") | ||
| } |
There was a problem hiding this comment.
This is the perf linter complaining about composing a string in a loop. Doing it with a string builder ends up being nicer anyway.
|
|
||
| // SpiceDB returns an error when no schema is defined, which is expected | ||
| assert.Error(t, err) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
testifylint likes require when making a NoError assertion, at least where possible. I think this was LLM code.
| if opts.RedactRelations { | ||
| switch t := allowedDirect.RelationOrWildcard.(type) { | ||
| case *core.AllowedRelation_Relation: | ||
| if t, ok := allowedDirect.RelationOrWildcard.(*core.AllowedRelation_Relation); ok { |
There was a problem hiding this comment.
This was a single-case switch, so we refactor to an if.
There was a problem hiding this comment.
These are all ported over relatively directly from SpiceDb.
tstirrat15
force-pushed
the
enable-more-linters
branch
2 times, most recently
from
1b31c4e to
5e4d461
Compare
tstirrat15
force-pushed
the
enable-more-linters
branch
from
5e4d461 to
997d52f
Compare
| } | ||
| } | ||
|
|
||
| func TestSchemaCompile(t *testing.T) { |
There was a problem hiding this comment.
I refactored these to be three separate tests because 1. you can't really use require.ErrorAs with a tabular test, at least in its current form (which may change in golang 1.26 but we're not there yet) and 2. one of the error assertions needed to be ErrorIs, not ErrorAs. There's a bit of duplicated code but I'm fine with that.
| // TODO: refactor the impl function to provide a pipe or buffer directly and delegate the input selection to | ||
| // another function | ||
| // | ||
| //nolint:tparallel // these tests can't be parallel because they muck around with the definition of os.Stdin. |
There was a problem hiding this comment.
but you kept the t.parallel in line 195
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #600 +/- ##
==========================================
+ Coverage 39.28% 41.80% +2.52%
==========================================
Files 37 37
Lines 5448 4774 -674
==========================================
- Hits 2140 1996 -144
+ Misses 3063 2523 -540
- Partials 245 255 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Makes linting in this repo stricter and brings it in line with what we use on SpiceDB.
This also separates out some of the changes from #597.
Changes
Will annotate.
Testing
Review. See that tests pass.