Fix lints #625

thanethomson · 2023-03-31T13:09:29Z

This touches a large number of files, but I believe it's necessary as part of our tech debt cleanup. There are so many functions littering the codebase where:

We have unused parameters and they should have been refactored away, but they weren't
We have unused parameters and they should be named according to proper Go conventions (e.g. in interface implementations where a particular function signature is required, but the variable names should either be left out or _ when unused)
We have redundant code (a whole bunch of redundant if statements and error checks, for example)
We use bad naming conventions for variables, like copy or len

I'm also tired of having the linter fail locally. And if we don't do this, it will all just rot even more.

This PR targets main, but should be applied across all the backport branches (which will, of course, require some conflict resolution, but I'm fine with doing that). I've left TODOs in some places where changes need to be made in follow-up PRs.

Commits are organized by package so that it's hopefully easier to review.

If you pick up on formatting changes, please see #604.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson · 2023-03-31T13:13:28Z

Makefile

@@ -231,7 +231,7 @@ format:

 lint:
 	@echo "--> Running linter"
-	@go run github.com/golangci/golangci-lint/cmd/golangci-lint run
+	@go run github.com/golangci/golangci-lint/cmd/golangci-lint@latest run


By not specifying latest here, the version of golangci-lint used locally will be whichever version is currently installed in your ${GOPATH}/bin, meaning that each of us will potentially have a different version and therefore experience different results when linting locally.

Using latest here makes it consistent for all of us.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson · 2023-03-31T13:23:21Z

.github/workflows/lint.yml

@@ -31,7 +31,7 @@ jobs:
            go.sum
      - uses: golangci/golangci-lint-action@v3
        with:
-          version: v1.51
+          version: latest


I think we should keep up with the latest golangci-lint releases, especially if we're enforcing this check in the lint target of the Makefile, to ensure consistency.

thanethomson · 2023-03-31T13:33:54Z

cmd/cometbft/commands/root.go

@@ -98,7 +98,8 @@ var RootCmd = &cobra.Command{
 }

 // deprecateSnakeCase is a util function for 0.34.1. Should be removed in 0.35
-func deprecateSnakeCase(cmd *cobra.Command, args []string) {
+// TODO(thane): Remove this across all releases


lasarojc

while I dislike some of the the practices pushed by the Lint, I all all for uniformity.
LGTM

blocksync/reactor.go

sergio-mena · 2023-04-04T17:19:22Z

p2p/test_util.go

@@ -178,11 +178,9 @@ func StartSwitches(switches []*Switch) error {
 func MakeSwitch(
 	cfg *config.P2PConfig,
 	i int,
-	network, version string,


Good catch!

sergio-mena · 2023-04-04T17:26:35Z

rpc/grpc/api.go

+func (bapi *broadcastAPI) Ping(context.Context, *RequestPing) (*ResponsePing, error) {
 	// kvstore so we can check if the server is up
 	return &ResponsePing{}, nil
 }

-func (bapi *broadcastAPI) BroadcastTx(ctx context.Context, req *RequestBroadcastTx) (*ResponseBroadcastTx, error) {
+func (bapi *broadcastAPI) BroadcastTx(_ context.Context, req *RequestBroadcastTx) (*ResponseBroadcastTx, error) {


We are setting the variable name to "_" in line 20, and removing the name altogether in line 15?
Can we treat the two cases the same way'

The convention in Go is that, if all function parameters are unused, then you can have unnamed parameters. But the moment that one of your parameters is used, they all have to be named, in which case the unused ones must be called _.

sergio-mena

Thanks for this @thanethomson
I like it because there is zero risk of breaking something and the changes are sparse enough not to hinder git-blame significantly.
One thought: since this is strictly refactoring, is it worth your time backporting it to other branches?

thanethomson · 2023-04-05T00:10:56Z

One thought: since this is strictly refactoring, is it worth your time backporting it to other branches?

If we don't, our local linter's going to fail when linting those branches, so I think I'll have to backport this.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

This touches a large number of files, but I believe it's necessary as part of our tech debt cleanup. There are so many functions littering the codebase where: 1. We have unused parameters and they should have been refactored away, but they weren't 2. We have unused parameters and they should be named according to proper Go conventions (e.g. in interface implementations where a particular function signature is required, but the variable names should either be left out or `_` when unused) 3. We have redundant code (a whole bunch of redundant `if` statements and error checks, for example) 4. We use bad naming conventions for variables, like `copy` or `len` I'm also tired of having the linter fail locally. And if we don't do this, it will all just rot even more. This PR targets `main`, but should be applied across all the backport branches (which will, of course, require some conflict resolution, but I'm fine with doing that). I've left TODOs in some places where changes need to be made in follow-up PRs. Commits are organized by package so that it's hopefully easier to review. If you pick up on formatting changes, please see #604. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit 111d252) # Conflicts: # consensus/state.go # rpc/core/abci.go # rpc/core/blocks.go # rpc/core/consensus.go # rpc/core/evidence.go # rpc/core/health.go # rpc/core/mempool.go # rpc/core/net.go # rpc/core/status.go # rpc/core/tx.go

This touches a large number of files, but I believe it's necessary as part of our tech debt cleanup. There are so many functions littering the codebase where: 1. We have unused parameters and they should have been refactored away, but they weren't 2. We have unused parameters and they should be named according to proper Go conventions (e.g. in interface implementations where a particular function signature is required, but the variable names should either be left out or `_` when unused) 3. We have redundant code (a whole bunch of redundant `if` statements and error checks, for example) 4. We use bad naming conventions for variables, like `copy` or `len` I'm also tired of having the linter fail locally. And if we don't do this, it will all just rot even more. This PR targets `main`, but should be applied across all the backport branches (which will, of course, require some conflict resolution, but I'm fine with doing that). I've left TODOs in some places where changes need to be made in follow-up PRs. Commits are organized by package so that it's hopefully easier to review. If you pick up on formatting changes, please see #604. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/uncl F438 og) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit 111d252) # Conflicts: # abci/client/grpc_client.go # abci/client/grpc_client_test.go # abci/client/socket_client.go # abci/client/socket_client_test.go # abci/cmd/abci-cli/abci-cli.go # abci/example/kvstore/kvstore.go # abci/example/kvstore/kvstore_test.go # abci/server/grpc_server.go # abci/tests/server/client.go # abci/types/application.go # blocksync/reactor.go # cmd/cometbft/commands/inspect.go # consensus/reactor_test.go # consensus/replay_stubs.go # consensus/replay_test.go # consensus/state.go # consensus/state_test.go # consensus/types/height_vote_set_test.go # inspect/inspect.go # inspect/inspect_test.go # mempool/v0/clist_mempool_test.go # node/node.go # node/setup.go # p2p/base_reactor.go # p2p/mock/peer.go # p2p/mock/reactor.go # p2p/peer_set_test.go # rpc/client/local/local.go # rpc/client/mock/client.go # rpc/core/abci.go # rpc/core/blocks.go # rpc/core/consensus.go # rpc/core/dev.go # rpc/core/evidence.go # rpc/core/health.go # rpc/core/mempool.go # rpc/core/net.go # rpc/core/status.go # rpc/core/tx.go # state/indexer/block/kv/util.go # state/state_test.go # state/txindex/kv/utils.go # statesync/reactor.go # statesync/reactor_test.go # store/store_test.go # test/e2e/app/app.go # test/e2e/runner/evidence.go # types/event_bus.go # types/validator_set.go

This touches a large number of files, but I believe it's necessary as part of our tech debt cleanup. There are so many functions littering the codebase where: 1. We have unused parameters and they should have been refactored away, but they weren't 2. We have unused parameters and they should be named according to proper Go conventions (e.g. in interface implementations where a particular function signature is required, but the variable names should either be left out or `_` when unused) 3. We have redundant code (a whole bunch of redundant `if` statements and error checks, for example) 4. We use bad naming conventions for variables, like `copy` or `len` I'm also tired of having the linter fail locally. And if we don't do this, it will all just rot even more. This PR targets `main`, but should be applied across all the backport branches (which will, of course, require some conflict resolution, but I'm fine with doing that). I've left TODOs in some places where changes need to be made in follow-up PRs. Commits are organized by package so that it's hopefully easier to review. If you pick up on formatting changes, please see #604. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit 111d252) # Conflicts: # .github/workflows/lint.yml # abci/client/grpc_client.go # abci/client/grpc_client_test.go # abci/client/socket_client.go # abci/client/socket_client_test.go # abci/cmd/abci-cli/abci-cli.go # abci/example/kvstore/kvstore.go # abci/example/kvstore/kvstore_test.go # abci/server/grpc_server.go # abci/tests/server/client.go # abci/types/application.go # blockchain/v0/reactor.go # cmd/cometbft/commands/inspect.go # cmd/cometbft/commands/light.go # consensus/reactor_test.go # consensus/replay_stubs.go # consensus/replay_test.go # consensus/state.go # consensus/state_test.go # consensus/types/height_vote_set_test.go # consensus/wal_generator.go # inspect/inspect.go # inspect/inspect_test.go # internal/test/validator.go # mempool/v0/clist_mempool_test.go # node/node.go # node/setup.go # p2p/base_reactor.go # p2p/mock/peer.go # p2p/mock/reactor.go # p2p/peer_set_test.go # p2p/switch_test.go # rpc/client/local/local.go # rpc/client/mock/client.go # rpc/core/abci.go # rpc/core/blocks.go # rpc/core/consensus.go # rpc/core/dev.go # rpc/core/evidence.go # rpc/core/health.go # rpc/core/mempool.go # rpc/core/net.go # rpc/core/status.go # rpc/core/tx.go # scripts/metricsgen/metricsgen.go # state/indexer/block/kv/util.go # state/state_test.go # state/txindex/kv/kv_test.go # state/txindex/kv/utils.go # statesync/reactor.go # statesync/reactor_test.go # store/store_test.go # test/e2e/app/app.go # test/e2e/runner/evidence.go # types/event_bus.go # types/validator_set.go

* Fix lints (#625) This touches a large number of files, but I believe it's necessary as part of our tech debt cleanup. There are so many functions littering the codebase where: 1. We have unused parameters and they should have been refactored away, but they weren't 2. We have unused parameters and they should be named according to proper Go conventions (e.g. in interface implementations where a particular function signature is required, but the variable names should either be left out or `_` when unused) 3. We have redundant code (a whole bunch of redundant `if` statements and error checks, for example) 4. We use bad naming conventions for variables, like `copy` or `len` I'm also tired of having the linter fail locally. And if we don't do this, it will all just rot even more. This PR targets `main`, but should be applied across all the backport branches (which will, of course, require some conflict resolution, but I'm fine with doing that). I've left TODOs in some places where changes need to be made in follow-up PRs. Commits are organized by package so that it's hopefully easier to review. If you pick up on formatting changes, please see #604. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit 111d252) # Conflicts: # consensus/state.go # rpc/core/abci.go # rpc/core/blocks.go # rpc/core/consensus.go # rpc/core/evidence.go # rpc/core/health.go # rpc/core/mempool.go # rpc/core/net.go # rpc/core/status.go # rpc/core/tx.go * Fix conflicts Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Thane Thomson <connect@thanethomson.com>

This touches a large number of files, but I believe it's necessary as part of our tech debt cleanup. There are so many functions littering the codebase where: 1. We have unused parameters and they should have been refactored away, but they weren't 2. We have unused parameters and they should be named according to proper Go conventions (e.g. in interface implementations where a particular function signature is required, but the variable names should either be left out or `_` when unused) 3. We have redundant code (a whole bunch of redundant `if` statements and error checks, for example) 4. We use bad naming conventions for variables, like `copy` or `len` I'm also tired of having the linter fail locally. And if we don't do this, it will all just rot even more. This PR targets `main`, but should be applied across all the backport branches (which will, of course, require some conflict resolution, but I'm fine with doing that). I've left TODOs in some places where changes need to be made in follow-up PRs. Commits are organized by package so that it's hopefully easier to review. If you pick up on formatting changes, please see #604. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments

thanethomson added 21 commits March 30, 2023 17:07

Ensure latest version of golangci-lint is used locally

e6b4ab9

Signed-off-by: Thane Thomson <connect@thanethomson.com>

abci: Fix lints

0b17a89

Signed-off-by: Thane Thomson <connect@thanethomson.com>

blocksync: Fix lints

b9afcbe

Signed-off-by: Thane Thomson <connect@thanethomson.com>

cmd: Fix lints

10026ff

Signed-off-by: Thane Thomson <connect@thanethomson.com>

consensus: Fix lints

1f578bc

Signed-off-by: Thane Thomson <connect@thanethomson.com>

evidence: Fix lints

caff451

Signed-off-by: Thane Thomson <connect@thanethomson.com>

inspect: Fix lints

526dc06

Signed-off-by: Thane Thomson <connect@thanethomson.com>

internal: Fix lints

bfe930b

Signed-off-by: Thane Thomson <connect@thanethomson.com>

libs: Fix lints

d829b94

Signed-off-by: Thane Thomson <connect@thanethomson.com>

mempool: Fix lints

73da1eb

Signed-off-by: Thane Thomson <connect@thanethomson.com>

node: Fix lints

538cbf1

Signed-off-by: Thane Thomson <connect@thanethomson.com>

p2p: Fix lints

9f3834c

Signed-off-by: Thane Thomson <connect@thanethomson.com>

privval: Fix lints

2961104

Signed-off-by: Thane Thomson <connect@thanethomson.com>

rpc: Fix lints

0023d28

Signed-off-by: Thane Thomson <connect@thanethomson.com>

scripts: Fix lints

c8dfef3

Signed-off-by: Thane Thomson <connect@thanethomson.com>

state: Fix lints

436243c

Signed-off-by: Thane Thomson <connect@thanethomson.com>

light: Fix lints

999862a

Signed-off-by: Thane Thomson <connect@thanethomson.com>

statesync: Fix lints

1214d5c

Signed-off-by: Thane Thomson <connect@thanethomson.com>

store: Fix lints

2244cdb

Signed-off-by: Thane Thomson <connect@thanethomson.com>

test/e2e: Fix lints

bcf0aee

Signed-off-by: Thane Thomson <connect@thanethomson.com>

types: Fix lints

81df873

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson commented Mar 31, 2023

View reviewed changes

ci: Use latest golangci-lint

d5ef43b

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson commented Mar 31, 2023

View reviewed changes

thanethomson marked this pull request as ready for review March 31, 2023 13:23

thanethomson requested a review from a team as a code owner March 31, 2023 13:23

thanethomson mentioned this pull request Mar 31, 2023

cmd: Remove snake case deprecation warning #626

Merged

3 tasks

thanethomson commented Mar 31, 2023

View reviewed changes

lasarojc approved these changes Mar 31, 2023

View reviewed changes

blocksync/reactor.go Outdated Show resolved Hide resolved

sergio-mena reviewed Apr 4, 2023

View reviewed changes

sergio-mena approved these changes Apr 4, 2023

View reviewed changes

thanethomson added the automerge label Apr 5, 2023

Merge branch 'main' into thane/fix-lints

7910eda

thanethomson removed the automerge label Apr 5, 2023

lint: Address review comment

7eed185

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson added the automerge label Apr 5, 2023

mergify bot merged commit 111d252 into main Apr 5, 2023

mergify bot deleted the thane/fix-lints branch April 5, 2023 10:55

mergify bot mentioned this pull request Apr 5, 2023

Fix lints (backport #625) #647

Merged

This was referenced Apr 5, 2023

Fix lints (backport #625) #648

Closed

Fix lints (backport #625) #649

Closed

faddat mentioned this pull request Oct 15, 2024

chore: use latest cometbft-db in v0.38.x #4296

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix lints #625

Fix lints #625

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix lints #625

Fix lints #625

Uh oh!

Conversation

Uh oh!

PR checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!