Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Staticcheck Should Be Considered For "All" Repository CI Processes #866

Closed
bonedaddy opened this issue Apr 2, 2020 · 5 comments
Closed
Labels
kind/enhancement A net-new feature or improvement to an existing feature

Comments

@bonedaddy
Copy link

staticcheck is a pretty awesome static analysis tool. It covers a wide variety of checks from simplification, missed error checks, overshadowed variables, etc... and it's also very configurable which means you can ignore lints you dont like.

In particular the lints for missed error checks, overshadowed variables, defers before error checks, are pretty useful. From my own experience it seems to lead to a healthier code base, and has identified on more than one occasion logic that would in edge cases cause bugs, as well as helping to figure out why a test was passing that shouldn't.

Implementing them in all repositories is certainly a bit excessive, but the great thing about staticcheck is that it can be implemented without any disruptions or needing to consider breaking changes.

Starting to get pretty sleepy as I write this so I'll show a couple of the "superficial" lints performed.

	f := func(ctx context.Context, p peer.ID) bool {
		if p == candidate {
			return true
		}
		return false
	}

Which could be changed to

	f := func(ctx context.Context, p peer.ID) bool {
		return p == candidate
	}

Now in this particular case whether this is an improvement is ultimately up to you. It can also catch things like:

select {
case <-ctx.Done():
}

Which can be simplified into:

<-ctx.Done()
@bonedaddy bonedaddy added the kind/enhancement A net-new feature or improvement to an existing feature label Apr 2, 2020
@marten-seemann
Copy link
Contributor

In quic-go, I'm using GolangCI-Lint. staticcheck can be run as part of that, in addition to a couple of other useful linters.

@Stebalien
Copy link
Member

I plan on slowly transitioning to CircleCI with https://circleci.com/orbs/registry/orb/ipfs/ci-go (we're doing the same in go-ipfs). That will give us:

  • Cross platform builds (ensures we at least build on all reasonable platforms).
  • lints
  • tests
  • benchmark against master (kind of broken at the moment)

@raulk
Copy link
Member

raulk commented May 12, 2020

We should adopt golangci-lint. Barring memory usage regressions in v1.24, it has been fantastic on https://github.com/testground/testground/.

@raulk
Copy link
Member

raulk commented May 12, 2020

CircleCI migration: #920.

@BigLep
Copy link
Contributor

BigLep commented Mar 29, 2021

Closing since we're moving to unified CI using GitHub Actions.

@BigLep BigLep closed this as completed Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants