chore: enable gocritic #2682
chore: enable gocritic #2682
Conversation
github-actions
bot
added
area/cli
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (44.66%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## main #2682 +/- ##
===========================================
- Coverage 79.43% 44.66% -34.77%
===========================================
Files 456 414 -42
Lines 47320 43897 -3423
===========================================
- Hits 37586 19603 -17983
- Misses 6971 22448 +15477
+ Partials 2763 1846 -917 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| intFromString := func(s string) (int, error) { | ||
| return strconv.Atoi(s) | ||
| } |
There was a problem hiding this comment.
This is the unlambda rule - basically don't wrap a function in a lambda if the lambda is exactly the same as the function.
There was a problem hiding this comment.
I don't like lambdas in general because they make reading stacktraces a pain in the behind
| switch { | ||
| case foundCount == 0: |
There was a problem hiding this comment.
Some if/elseif/else rewriting to cases
internal/graph/lookupresources3.go
Outdated
| cursor := (*index.dbCursor) | ||
| return dsIndexPrefix + index.chunkID + ":" + cursor.String(), nil |
There was a problem hiding this comment.
gocritic was complaining that you could get rid of the deref here, except that you can't because the compiler doesn't correctly recognize the interface. Separating the assignment fixes things.
| newArgs := append(args, strValue) | ||
| newArgs := make([]any, 0, len(args)+len(strValue)) | ||
| newArgs = append(newArgs, args) | ||
| newArgs = append(newArgs, strValue) |
There was a problem hiding this comment.
I think these are functionally equivalent, but gocritic doesn't like it when you use append and assign to a new slice, so this makes it more explicit.
There was a problem hiding this comment.
This change is the problem. I'm not sure why it's semantically different.
| datastore.WithRequestHedgingEnabled(false), | ||
| ) | ||
| if err != nil { | ||
| log.Fatalf("unable to start memdb datastore: %s", err) |
There was a problem hiding this comment.
It was complaining here that log.Fatalf prevents defer blocks from running. t.Fatalf should be equivalent and also work around that.
| asTuples := slicez.Map(testfixtures.StandardRelationships, func(item string) tuple.Relationship { | ||
| return tuple.MustParse(item) | ||
| }) |
There was a problem hiding this comment.
Same unlambda thing here.
tstirrat15
force-pushed
the
enable-gocritic
branch
from
80b1618 to
6f258a0
Compare
| } | ||
|
|
||
| return dsIndexPrefix + index.chunkID + ":" + (*index.dbCursor).String(), nil | ||
| return dsIndexPrefix + index.chunkID + ":" + index.dbCursor.RelationshipReference.String(), nil |
There was a problem hiding this comment.
I'm not entirely sure how this deref worked previously - this both makes things a little clearer and also satisfies gocritic.
There was a problem hiding this comment.
Except that it doesn't work 🤔
There was a problem hiding this comment.
does this explain the broken tests or are you talking about something else?
There was a problem hiding this comment.
I thought this was the test failure but it's not 🤔
|
|
||
| checkedUpdate = true | ||
| } else { | ||
| // we wait for a checkpoint right after the update. Checkpoints could happen at any time off band. |
There was a problem hiding this comment.
the comment should be moved too
tstirrat15
force-pushed
the
enable-gocritic
branch
from
6f258a0 to
5db047d
Compare
Description
Getting close to done with linting things! This enables
gocriticacross the repo.Changes
Will annotate.
Testing
Review. See that tests pass.