[#310] upgrade to latest golangci-lint and enable new linters #311
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sahilm
force-pushed
the
sahilm/upgrade-to-latest-golangci-lint
branch
from
1e014dc
to
3de9e13
Compare
This problem has been fixed in go 1.22: https://go.dev/blog/loopvar-preview
sahilm
force-pushed
the
sahilm/upgrade-to-latest-golangci-lint
branch
from
cabe2cd
to
4621fbb
Compare
In most cases we would rather use assert.Error so that the test keeps going and gives us more information. This linter doesn't seem to be valuable enough to enforce.
sahilm
changed the title
See warnings like: level=warning msg="[config_reader] The output format `github-actions` is deprecated, please use `colored-line-number`"
sahilm
changed the title
sahilm
commented
Jun 26, 2024
| @@ -292,10 +292,10 @@ func (c *Checker) replaceChunk(chunk *table.Chunk) error { | |||
| c.recopyLock.Lock() | |||
| defer c.recopyLock.Unlock() | |||
|
|
|||
| if _, err := dbconn.RetryableTransaction(context.TODO(), c.db, false, dbconn.NewDBConfig(), deleteStmt); err != nil { | |||
| if _, err := dbconn.RetryableTransaction(ctx, c.db, false, dbconn.NewDBConfig(), deleteStmt); err != nil { | |||
There was a problem hiding this comment.
Hope there's no unintended side effects here. I don't think this is a problem but want to highlight this change. It's to satisfy the contextcheck linter.
There was a problem hiding this comment.
It looks good to me. I think this is a code improvement.
sahilm
commented
Jun 26, 2024
| @@ -2368,7 +2368,7 @@ func TestSkipDropAfterCutover(t *testing.T) { | |||
| var tableCount int | |||
| err = m.db.QueryRow(sql).Scan(&tableCount) | |||
| assert.NoError(t, err) | |||
| assert.Equal(t, tableCount, 1) | |||
| assert.Equal(t, 1, tableCount) | |||
There was a problem hiding this comment.
To fix testifylint: expected-actual
morgo
approved these changes
Jun 26, 2024
| @@ -292,10 +292,10 @@ func (c *Checker) replaceChunk(chunk *table.Chunk) error { | |||
| c.recopyLock.Lock() | |||
| defer c.recopyLock.Unlock() | |||
|
|
|||
| if _, err := dbconn.RetryableTransaction(context.TODO(), c.db, false, dbconn.NewDBConfig(), deleteStmt); err != nil { | |||
| if _, err := dbconn.RetryableTransaction(ctx, c.db, false, dbconn.NewDBConfig(), deleteStmt); err != nil { | |||
There was a problem hiding this comment.
It looks good to me. I think this is a code improvement.
morgo
approved these changes
Jun 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's a degree of subjectivity to linting. Each commit represents an atomic decision and is open to change/discussion. Please refer to the commit in question if you'd like to discuss/change any decision.
Resolves #310