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

golangci-lint configuration file #895

Merged
merged 1 commit into from May 23, 2020
Merged

Conversation

sam-github
Copy link
Collaborator

golangci-lint can run all the currently enabled linters, and as far as I
can tell, does it in under 5 seconds as opposed to over 180 seconds
(compare time make cilint and time make lint).

Some of the linters that are listed in the "enabled" section but
commented out looked like a good idea to me, and fairly low hanging
fruit to fix, but they are not passing at the moment.

All the linters covered in the current Makefile are run.

TODO:

  • replace lint target in Makefile with golangci-lint
  • remove .github/workflow/errcheck.yml

Just a suggestion. It might be much faster, and yet do the same thing, win-win. I did some fixing of things gocritic didn't like (its a big fan of switch statements over if/if else/if else/else series), but then stopped and disabled it. Some of the sec warnings seem not applicable to wtf given that it does shell out to user-defined commands at times, so enabling gosec would involve whitelisting some of the warnings. Etc.

On the down side, since this is passing, I won't see the line-annotation feature that is supposed to be enabled... maybe I should enable a couple of the failing linters temporarily in the PR, so that feature is visible?

@sam-github
Copy link
Collaborator Author

Its possible (see golangci/golangci-lint#825) that this is because go build (no args) tries to make a ./wtf executable, but there is already a ./wtf/ directory.

golangci-lint can run all the currently enabled linters, and as far as I
can tell, does it in under 5 seconds as opposed to over 180 seconds
(compare `time make cilint` and `time make lint`).

Some of the linters that are listed in the "enabled" section but
commented out looked like a good idea to me, and fairly low hanging
fruit to fix, but they are not passing at the moment.

All the linters covered in the current Makefile are run.

TODO:
- replace lint target in Makefile with golangci-lint
- remove .github/workflow/errcheck.yml
@sam-github
Copy link
Collaborator Author

The inline annotations are nice (the existing integration likely has them, too, that's not an advantage).

Turns out it was just a ./... arg that's needed in the CI action (though not locally).

@sam-github
Copy link
Collaborator Author

sam-github commented May 16, 2020

Times for runs can be seen below. golangci-lint was 1m, imports check 51 sec, the others are 2m+. A small win.

@senorprogrammer
Copy link
Collaborator

senorprogrammer commented May 17, 2020

This will be great on the CI system. For local installs we'll need to ensure that the user has it installed. What do you think of changing make lint to check that golangci-lint is present. If it is, use that. If not, inform on where to get it (https://golangci-lint.run/usage/install/#local-installation) and fall back to the old method.

That way we don't need two lint commands.

@senorprogrammer
Copy link
Collaborator

senorprogrammer commented May 17, 2020

And this change to the original lint command speeds it up quite a bit, making it only 7x slower than golangci-lint (the original version is 20x slower than this PR on my machine):

lint:
	@echo "\033[35mhttps://github.com/kisielk/errcheck\033[0m"
	errcheck ./app ./cfg ./flags ./help ./logger ./modules/... ./utils ./view ./wtf
	errcheck ./main.go

	@echo "\033[35mhttps://golang.org/cmd/vet/k\033[0m"
	go vet ./app ./cfg ./flags ./help ./logger ./modules/... ./utils ./view ./wtf
	go vet ./main.go

	@echo "\033[35m# https://staticcheck.io/docs/k\033[0m"
	staticcheck ./app ./cfg ./flags ./help ./logger ./modules/... ./utils ./view ./wtf 
	staticcheck ./main.go

	@echo "\033[35m# https://github.com/mdempsky/unconvert\033[0m"
	unconvert ./...

@sam-github
Copy link
Collaborator Author

Yes, running the tool against multiple inputs simultaneously helps the current stuff a lot, its basically the optimization that golangci-lint does.

The current lint target assumes errcheck and staticcheck were installed globally, the cilint target assumes golantlint-ci is installed globally, its the same situation, isn't it? I'm pretty sure when I first tried to run make lint, I just looked up the name of the tools from the comments in the Makefile, and did a manual got get ... on them, then re-ran.

The build tool situation with go modules seems a bit obscure ATM, from golang/go#25922 I've found and tried https://github.com/go-modules-by-example/index/blob/master/010_tools/README.md, but I still needed a setup style target:

setup:
	GOBIN=$$PWD/bin go install github.com/golangci/golangci-lint

# Make the obvious lint target depend on setup, so it works "out of the box"
lint: setup just-lint

# If you have done setup already, don't repeat it, its slow
just-lint:
        ./bin/golangci-lint run 

Other than an explicit setup target, I don't see a way of ensuring dev-time tools are available. Adding a tools.go to the above seems to be a way to ensure exact versions of the tools are installed, which could be a big deal for a long-running large project that needs stable builds over years, but not so much here.

@@ -86,6 +86,9 @@ install:
@echo "${APP} installed into ${INSTALLPATH}"

## lint: runs a number of code quality checks against the source code
cilint:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should have mentioned, I called this cilint just so I could leave the original lint target in. Its a poor name, given that ci doesn't use it, it just uses the action. Oops.

I think the cilint target should be renamed lint, and the (current) lint target deleted.

@senorprogrammer
Copy link
Collaborator

That’s an excellent point about the current one, I’d forgotten they’re not go standard. Let’s go straight with golintci.

@sam-github
Copy link
Collaborator Author

Btw, I don't use vscode, but it has golangci-lint integrations: https://golangci-lint.run/usage/integrations/, might be useful (I'm not sure if the standalone tools also do that).

@sam-github
Copy link
Collaborator Author

ok, give me a chance to tidy this up a bit, I'll add at least a couple comments so people know how to do the setup.

Actually, WDYT, setup target, or just a comment saying "go get it"? I suspect for vscode integration it might need to be global anyhow.

@senorprogrammer
Copy link
Collaborator

Go with the simplest to implement option at this stage. I suspect 99% of contributors don’t use the make tooling so having it on CI is really what matters

steps:
- uses: actions/checkout@v2
- name: golangci-lint
uses: golangci/golangci-lint-action@v0.2.0
Copy link

Choose a reason for hiding this comment

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

JFYI we've launched v1 with support of caching and without slow docker image pulling, it works much faster

@senorprogrammer senorprogrammer merged commit 86c7e12 into wtfutil:master May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants