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

Add more linters to our CI pipeline #326

Merged
merged 4 commits into from Feb 25, 2022

Conversation

sebasslash
Copy link
Contributor

@sebasslash sebasslash commented Feb 22, 2022

Description

As part of our 1.0 efforts, we're trying to enforce best Go practices and improve consistency in our codebase. Our current CI pipeline is in need of much improvement allowing for inconsistencies, redundancies, and errors to go unnoticed. This PR enables several linters can serve as beneficial tools in our refactoring efforts:

goconst

One thing we noticed across our codebase were duplicate error messages. Goconst enables us to find these duplicate strings that can be refactored into a constant. In this case, commonly thrown errors should be refactored into a constant that lives in errors.go

Screen Shot 2022-02-16 at 5 08 56 PM

gocritic

This linter will handle the brunt of our linter pipeline enforcing opinionated best practices for go. I've enabled several tags for gocritic:

  • diagnostic - perform diagnostic checks to find potential programming errors.
  • opinionated - enforces gocritic's opinionated style guide.
  • performance - find minor performance issues that can become bottlenecks.

Some interesting errors returned:

  • Update and Create options are considered "heavy" in size and gocritic recommends we pass the parameter as a pointer.
  • Some of our variable names are shadow imports of packages, i.e var url string or var errors []string
  • Some of our regular expressions used in our validations can be simplified
  • If/else chains rewritten as switch statements
  • Comment formatting issues

Read more in the docs

misspell

Often times grammar mistakes make their way unnoticed into our documentation. While a simple spelling mistake won't necessarily hamper the development experience, with 1.0 we'd like to begin to enforce high documentation quality and that begins with the little things :) misspell will help us find these unnoticed buggers.

unparam

unparam will help us find function parameters that are unused.

External links

@sebasslash sebasslash requested a review from a team as a code owner Feb 22, 2022
@sebasslash sebasslash force-pushed the sebasslash/linters-oh-my branch 2 times, most recently from 43dbdec to 6fd90d8 Compare Feb 22, 2022
Copy link
Collaborator

@brandonc brandonc left a comment

I agree with the new infractions so this seems like a good set of linters. How can we make it easy to run these in development? Pre-commit or something?

.golangci.yml Outdated
- gocritic
- unparam
- misspell
Copy link
Collaborator

@brandonc brandonc Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a linter registry somewhere? How does it know about these? Can we add github links as above?

Copy link
Contributor Author

@sebasslash sebasslash Feb 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These linters are included with golangci-lint and are disabled by default -- err, at least we're first disabling all and then explicitly enabling which ones we want. More information about which linters are included and enabled.

Alternatively, you can run golangci-lint linters in the root of your project's directory to see the linters that have been enabled for the project.

.golangci.yml Outdated
- errcheck #https://github.com/kisielk/errcheck
- stylecheck #https://github.com/dominikh/go-tools/tree/master/stylecheck
- revive #golint is deprecated and golangci-lint recommends to use revive instead https://github.com/mgechev/revive
#other deprecated lint libraries: maligned, scopelint, interfacer
- gocritic
Copy link
Contributor

@uturunku1 uturunku1 Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the issues that gocritic is listing below for hugeParam is nice! It agrees with the type of refactoring we did previously around ListOptions and pointers

@uturunku1
Copy link
Contributor

uturunku1 commented Feb 23, 2022

Will you implementing the fixes requested by the new lint libraries in this PR?

@uturunku1 uturunku1 force-pushed the sebasslash/linters-oh-my branch from 6fd90d8 to 11e07db Compare Feb 24, 2022
.golangci.yml Show resolved Hide resolved
@sebasslash sebasslash merged commit df4ec36 into releases-1.0.x Feb 25, 2022
4 checks passed
@sebasslash sebasslash deleted the sebasslash/linters-oh-my branch Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants