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 support for golangci-lint #2182

Merged
merged 6 commits into from Mar 24, 2019

Conversation

@micahcoffman
Copy link
Contributor

commented Mar 19, 2019

Throwing this PR out here to get a discussion started on how we want golangci-lint defaults to look. It's a bit different than gometalinter, but a lot of the gometalinter code copies over nicely. Right now I set the defaults on autosave to be all the "fast" linters and it works pretty well. This seems to be how the other integrations with golangci-lint are working.

endfor

for linter in go#config#GolangciLintDisabled()
let cmd += ["--disable=".linter]

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 19, 2019

Author Contributor

--disable conflicts with the --disable-all flag when run together, so need to hash out some more things around that

let cmd = [bin_path]
let cmd += ["run"]
let cmd += ["--print-issued-lines=false"]
let cmd += ["--fast"]

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 19, 2019

Author Contributor

this flag makes it so only the "fast" linters run, probably don't want to hardcode this

endfunction

function! go#config#GolangciLintDisabled() abort
return get(g:, "go_golangci_lint_disabled", ['megacheck'])

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 19, 2019

Author Contributor

golangci-lint gives a nasty warning message about not being able to run megacheck on code that can't compile unless megacheck is explicitly disabled. there's probably a better way of getting that warning message to go away

@bhcleek
Copy link
Collaborator

left a comment

Thanks for getting this started. Overall, this is looking good. I think, though, that there are some improvements that can be made, especially since gometalinter is on life support and we want to fully replace it in vim-go.

What do you think about not adding any new commands, but simply reshaping the metalinter stuff to use golangci-lint instead? Specifically:

  1. use the options that currently apply to gometalinter (e.g. the g:go_metalinter_deadline, g:go_metalinter_autosave, etc.)
  2. Why isn't s:lint_job being used for both gometalinter and golangci-lint jobs? It looks like it should be sufficient, and you'd save introducing a nearly identical function even if we do end up keeping both gometalinter and golangci-lint.
  3. I don't see a need for a user to use both gometalinter and golangci-lint, even if we do keep support for both. Supporting both, but only using one at a time provides an additional opportunity to reduce the amount of code that this PR introduces. A single new option, g:go_metalinter could be used to control whether gometalinter or golangci-lint is used. A new function, s:metalintercmd, that gets called from go#lint#Gometa to setup the necessary command would be all that's necessary.

What do you think?

@micahcoffman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@bhcleek I'm +1 on all those ideas, just wasn't sure where you all wanted to head with gometalinter, so I spun this up without touching any of that code! I can start in on some of those improvements.

#3 is a great idea

@bhcleek

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

I appreciate you being careful 🙇

FWIW, gometalinter is deprecated and soon to be archived: https://github.com/alecthomas/gometalinter#go-meta-linter. By providing an option to allow a user to choose between gometalinter and golangci-lint, we can be assured that it will be very easy to remove the unnecessary bits for gometalinter once we're sure everything works as expected with golangci-lint.

Because gometalinter and golangci-lint are a meta linter, I'm comfortable with using golangci-lint via :GoMetaLinter, because it's not :GoGoMetaLinter 😄

@micahcoffman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@bhcleek I updated the code to your suggestions, defaulting to gometalinter as the g:go_metalinter so no one's config breaks. Take a look and let me know what you think. I'd still like to take a few to consider the best default flags for golangci-lint.

@bhcleek
Copy link
Collaborator

left a comment

This looks much better! Thank you for making the changes.

Since you want to take some time to make sure you're happy with the defaults, let me know when you're happy with the PR.

let l:default_enabled = ["vet", "golint"]

if go#config#Metalinter() == "golangci-lint"
let l:default_enabled = ["deadcode", "errcheck", "ineffassign", "structcheck", "typecheck", "varcheck"]

This comment has been minimized.

Copy link
@bhcleek

bhcleek Mar 20, 2019

Collaborator

Why have you chosen these as the defaults?

At a minimum, I'd expect the defaults for golangci-lint to at least use golint and govet to maintain parity with gometalinter for now.

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 20, 2019

Author Contributor

These are the "fast" defaults of golangci-lint https://github.com/golangci/golangci-lint#enabled-by-default-linters. There's also a --fast flag available to use, but we would need to pass it in as another config if we wanted to use it because the non-autosave command shouldn't be restricted to "fast" linters.

I personally use golint, but it's not part of their defaults, so I didn't throw it in. I would be happy putting it in.

Their govet seems a bit odd to be honest, I haven't gotten it to mimic the output of a regular go vet ./.... It seems like they do exclude duplicate checks between linters, so typecheck which is "fast" default takes over some of the govet behaviors. And then their govet just does some other source code analysis.

This comment has been minimized.

Copy link
@bhcleek

bhcleek Mar 20, 2019

Collaborator

Yeah, let's put in golint.

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 20, 2019

Author Contributor

added

let l:default_enabled = ["vet", "golint", "errcheck"]

if go#config#Metalinter() == "golangci-lint"
let l:default_enabled = ["deadcode", "errcheck", "ineffassign", "structcheck", "typecheck", "varcheck", "gosimple", "govet", "staticcheck", "unused"]

This comment has been minimized.

Copy link
@bhcleek

bhcleek Mar 20, 2019

Collaborator

A similar question here: why have you chosen these as the defaults?

At a minimum, I'd expect the defaults for golangci-lint to at least use golint, govet, and errcheck to maintain parity with gometalinter.

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 20, 2019

Author Contributor

these are just straight from the golangci-lint defaults https://github.com/golangci/golangci-lint#enabled-by-default-linters

This comment has been minimized.

Copy link
@bhcleek

bhcleek Mar 20, 2019

Collaborator

That makes sense; I'm just considering whether vim-go should try to maintain parity for the default linters regardless of whether it's using gometalinter or golangci-lint or whether it's ok to vary the default linters depending on which metalinter is being used.

if go#util#has_job()
let include = [printf('--include=^%s:.*$', expand('%:p:t'))]
endif
let cmd += include

This comment has been minimized.

Copy link
@bhcleek

bhcleek Mar 20, 2019

Collaborator

How can we ensure that only messages for the active buffer are included when using golangci-lint?

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 20, 2019

Author Contributor

they unfortunately don't have an include like gometalinter. i'll take another look at their flag options to see if there's something we can do

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 20, 2019

Author Contributor

figured this out, golangci-lint can be run per file so I added f574b1f

@micahcoffman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@bhcleek I'm ok with this PR as is now. I'm in favor of using the defaults + golint that golangci-lint specifies because of the exclusions between linters that they do behind the scenes. Using only the linters that were turned on for gometalinter won't yield the same results in golangci-lint. The config options also let people use the ones they want anyway.

This PR offers the same amount of support as the other editor integrations with golangci-lint, and i'm happy to help build on this support if people want more out of it in the future. If you're +1 on this PR now as is, let me know and i'll fixup these commits into one and then you can merge away.

let goargs = [expand('%:p:h')]
else
let goargs = [expand('%:p')]
endif
else
let goargs = a:000
endif

This comment has been minimized.

Copy link
@bhcleek

bhcleek Mar 20, 2019

Collaborator

This block changes things somewhat. When a:autosave is not set, then both gometalinter and golangci-lint should be passed the directory. When a:autosave is set then golangci-lint should be passed the file to check.

This comment has been minimized.

Copy link
@bhcleek

bhcleek Mar 20, 2019

Collaborator

To be clear, gometalinter also needs to be passed the file to check when a:autosave is set; that already happens for gometalinter a little further down the file. Perhaps this block could be restored to its previous glory, and the section below that ensure that gometalinter is given the --include option when a:autosave is set could be extended to append the file path to goargs when golangci-lint is used.

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 20, 2019

Author Contributor

Cool, that makes sense to me. I did this 2461989, do you think it could just be let goargs = [expand('%p')] instead of what I put in there? What all do we expect goargs could be when autosave is enabled?

@micahcoffman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

I also updated errorformat to include a better format for golangci-lint. It's slightly different than gometalinter.


function! go#config#MetalinterGrepNew() abort
return get(g:, "go_metalinter_grep_new", 0)
endfunction

This comment has been minimized.

Copy link
@bhcleek

bhcleek Mar 20, 2019

Collaborator

Neither go#config#MetalinterConfigEnabled nor go#config#MetalinterGrepNew is needed. vim-go doesn't need to support every possible option. If people want an escape hatch, there's already one in the form of g:go_metalinter_command.

This comment has been minimized.

Copy link
@micahcoffman

micahcoffman Mar 20, 2019

Author Contributor

Sorry, agreed, I didn't intend to push that. I'm playing around with the best way to interact with the golangci-lint config file. Config values are trumping flags which is a bit different than I expected.

This comment has been minimized.

Copy link
@bhcleek

bhcleek Mar 20, 2019

Collaborator

let's not worry about it; there's an escape hatch, and people can use the config as they see fit.

@bhcleek

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

@bhcleek I'm ok with this PR as is now.

Once you remove those 85d4505 and squash all these commits, let me know. Do you mind if I push to your branch to update docs and make some small tweaks before merging?

add support for golangci-lint
add g:go_metalinter config option and default to gometalinter

add golint to golangci-lint defaults

only include golangci-lint messages for active buffer

use disable-all behavior like gometalinter

set goargs to filepath for golangci when autosave is set

update errorformat for golangci-lint

@micahcoffman micahcoffman force-pushed the micahcoffman:golangci_linter_support branch from 85d4505 to 8c4a062 Mar 20, 2019

@micahcoffman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Should be good to go. You're welcome to push to my branch.

@bhcleek bhcleek changed the title WIP: add support for golangci-lint add support for golangci-lint Mar 21, 2019

@bhcleek

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

This looks great! Thank you for the contribution.

I'll make some small tweaks, update docs, and run with this locally for a while before I merge it.

bhcleek added some commits Mar 21, 2019

remove g:go_metalinter in favor of g:go_metalinter_command
Extend g:go_metalinter_command to control whether `gometalinter` or
`golangci-lint` is used and remove g:gometalinter so that the
configuration is more similar to other options (e.g. g:go_fmt_command).
create parity between default linters for gometalinter and golangci-lint
Configure the default linters used for golangci-lint to be faithful to
the default linters configured for gometalinter to minimize disruption
to the user experience.

@bhcleek bhcleek force-pushed the micahcoffman:golangci_linter_support branch from 0bdf470 to a62c7ce Mar 22, 2019

@bhcleek bhcleek merged commit 2774621 into fatih:master Mar 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

bhcleek added a commit that referenced this pull request Mar 24, 2019

@tommyknows

This comment has been minimized.

Copy link

commented Mar 25, 2019

Hi,
I just updated to use golanglint-ci and have a question;
right now, when enabling specific linters (for example typecheck) I get an error when having declarations outside the file that's currently in my buffer.
This is because the linter only checks one file, and not the whole package.
Is there an option to lint the whole package instead?

> golangci-lint run --disable-all --enable 'typecheck' pkg/x/y.go
pkg/x/y.go:71:2: undeclared name: `procRequests` (typecheck)
        procRequests.Inc()
        ^
> golangci-lint run --disable-all --enable 'typecheck' pkg/x/
>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.