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 linters #1107

Closed
wants to merge 3 commits into from
Closed

Add linters #1107

wants to merge 3 commits into from

Conversation

sagikazarmark
Copy link
Contributor

I gathered my strength and made the changes requested in #967 as a separate PR.

Before I go ahead and fix all those lint failures, I'd like to agree on the specific rules that we want to turn on. Right now I left everything on default mode.

Note: golint is deprecated. revive seems to be the alternative.

Fixes #967

Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
@sagikazarmark sagikazarmark marked this pull request as draft June 16, 2021 09:35
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
Copy link
Member

@ChrisHines ChrisHines left a comment

Choose a reason for hiding this comment

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

Should linting be a separate build workflow from running tests?

go vet ./...
bin/revive ./...
bin/staticcheck ./...
gofmt -l -s -e . | grep .go && exit 1
Copy link
Member

Choose a reason for hiding this comment

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

gofmt formatting can—and has, more than oncechange between versions of Go, how will we handle that when our builds straddle the versions where a change is made?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the linters Peter listed in his last comment. I don't mind dropping gofmt.

@sagikazarmark
Copy link
Contributor Author

Should linting be a separate build workflow from running tests?

Yeah, that's a good idea.

@sagikazarmark sagikazarmark mentioned this pull request Jun 18, 2021
@peterbourgon
Copy link
Member

Not really down with revive, specifically the notion that it's a framework for your own ruleset — that's explicitly what linters should not be! 😉 golint is still as effective as it ever was, let's stick with that. I could also be convinced to switch to mvdan/gofumpt but it will generate a ton of work on first run, I'm sure.

But what is motivating this PR? Is it a specific need, or general housekeeping? If it's general housekeeping, let's maybe wait until after a release, there's a lot of stuff in flight right now, don't you think?

@ChrisHines
Copy link
Member

I'm not sure how much we even need golint if we're using staticcheck, I suspect there would be a lot of duplicate checks between the two. But I'm not opposed to golint.

What do either of you think of using https://github.com/reviewdog/reviewdog to automate posting linter results as PR comments? I've used it for several years on work projects and find it reduces work for reviewers while simultaneously providing good feedback for contributors.

@sagikazarmark
Copy link
Contributor Author

I guess the first thing is to settle on the list of tools:

  • go vet
  • staticcheck
  • gofmt -> gofumpt?
  • revive -> golint?

But what is motivating this PR?

I think originally I submitted it about a year ago because of some "nit: do XY because coding style" kind of reviews. Linters can automate that and reduce turnaround time. Obviously, introducing new linters require some housekeeping.

What do either of you think of using https://github.com/reviewdog/reviewdog

I haven't used reviewdog before, but I'm familiar with it. I think it's a good idea, but I think GitHub Actions has a similar feature: it can decode the output (we can register decoders) and annotate specific lines. Not sure if it actually comments on the PR though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants