Skip to content

Add golangci-lint plugin #83

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

Merged
merged 4 commits into from
Mar 8, 2019
Merged

Add golangci-lint plugin #83

merged 4 commits into from
Mar 8, 2019

Conversation

cixtor
Copy link
Member

@cixtor cixtor commented Feb 28, 2019

Project inspired by alecthomas version [1] which unfortunately doesn’t works very well with the latest version of golangci-lint nor SublimeText and hasn’t been updated during the last 9+ months. He also announced a few days ago that he’s going to deprecate “gometalinter” [2] another popular linter in the Go (golang) community, so I jumped right away to offer an alternative.

Note: I want to transfer this linter to the SublimeLinter organization.

[1] https://github.com/alecthomas/SublimeLinter-contrib-golang-cilint
[2] alecthomas/gometalinter#590

Project inspired by alecthomas version [1] which unfortunately doesn’t
works very well with the latest version of golangci-lint nor SublimeText
and hasn’t been updated during the last 9+ months. He also announced a
few days ago that he’s going to deprecate “gometalinter” [2] another
popular linter in the Go (golang) community, so I jumped right away to
offer an alternative.

Note: I want to transfer this linter to the SublimeLinter organization.

[1] https://github.com/alecthomas/SublimeLinter-contrib-golang-cilint
[2] alecthomas/gometalinter#590
I spoke with @alecthomas about the possibility to merge both projects
since his version is older than mine, and people are probably already
using it. My version contains better error and warning parsing, and
works better with the latest version of golangci-lint. He agree to
review and maybe accept a pull-request with my version of the code, so
I’m changing this pull-request to link to his repository rather than
mine.
@braver
Copy link
Member

braver commented Mar 1, 2019

Ok 👍🏻 We don’t list his version, so we could add yours. You could also try to get @alecthomas to give you access to his and collaborate on it. That way there aren’t two competing projects on GitHub trying to do the same thing. That way people will be able to more easily find and contribute to the right package. Or propose to move his repo into the SublimeLinter org, and then we can also give you access make the changes it needs.
So, can you try and spearhead this and resolve the duplication issue in some way?

@cixtor
Copy link
Member Author

cixtor commented Mar 1, 2019

can you try and spearhead this and resolve the duplication issue in some way?

Thank you, I’ll talk to him.

@alecthomas
Copy link

Hi. I'm the author of the original package, but I no longer maintain it. I'd like to propose that @cixtor's package become the "blessed" one and I'll decommission mine.

cixtor added 2 commits March 5, 2019 14:17
Alec Thomas agreed to transfer the maintenance of the SublimeLinter
plugin for golangci-lint to me on Mar 5, 2019 [1]. The other version of
the code will be decommissioned and this one will become the canonical
repository for the linter.

[1] #83 (comment)
@braver
Copy link
Member

braver commented Mar 6, 2019

Excellent, works for me.

@cixtor please create a 1.0.0 release in your repository.

@cixtor
Copy link
Member Author

cixtor commented Mar 6, 2019

@braver Done! Let me know if this works [1].

I also included a “sublime-package” for convenience, but I’m not sure if Package Control prefers that over plain Zip archives. People who don’t use Package Control may prefer to simply drag and drop this file into their “Installed Packages” folders (although, they probably need Package Control anyway to install SublimeLinter, so maybe this file is redundant).

[1] https://github.com/cixtor/SublimeLinter-golangcilint/releases/tag/1.0.0

@braver
Copy link
Member

braver commented Mar 7, 2019

Yeah you don’t need to make that file. If people want to do a manual install they can just unzip a download into the Packages dir. Installed Packages is best left to package control. In fact, package control will create the zips for you from source by exporting the tag, you don’t have to attach the zips to the release.

@braver
Copy link
Member

braver commented Mar 7, 2019

@cixtor can you transfer the repo to me so I can move it into the org?

@kaste perhaps you want to have a look at what this linter does and give some feedback?

@kaste
Copy link
Contributor

kaste commented Mar 7, 2019

Honestly, I don't want it in the org if it's doesn't follow the official API, well at least a bit. 😁

I think at least executing the command and parsing should be separated. For parsing implementing find_errors(output) should be 👌. We should not parse json -> format as string -> reparse using regexes. That doesn't make sense. Instead (usually) in find_errors you parse the json and yield one error at a time, preferrable using the new LintMatch(). 'stylelint' will be a great example ifff @braver finishes his pull request 😉

We don't print, we log.

We usually hate tempdir generation, but 🤷‍♂️ isn't the linter generally too slow for running in background?

@cixtor
Copy link
Member Author

cixtor commented Mar 7, 2019

I can address these concerns, but I need more time to read SublimeLinter’s code since the official documentation is very scarce on details. I agree that the “json -> string -> regex” part is weird, I was trying to make sense of how SublimeLinter works and this was the only obvious way to proceed at the moment.

We usually hate tempdir generation […]

I hate it too, but right now it’s the only way to make golangci-lint work. If the changes in the file are not saved, for example, the tool will not be able to process the file that the user is modifying, instead it will just process the actual content of the file (which may not contain warnings or errors). This is in my TODO list of things to improve, but again, it seems to be the only way to make it work for now.

[…] isn't the linter generally too slow for running in background?

By default, golangci-lint runs 31 different linters (if they are installed), but this SublimeLinter plugin executes the tool with a --fast flag that turns the slowest linters off. I went ahead and tested this using the 25 fastest linters against one of the biggest repositories I have and here are the results (time in seconds):

LINTER Run N1 Run N2 Run N3
gochecknoinits 0.547s 0.559s 0.574s
nakedret 0.555s 0.563s 0.551s
goconst 0.556s 0.559s 0.573s
gocyclo 0.557s 0.561s 0.548s
gochecknoglobals 0.562s 0.555s 0.571s
scopelint 0.567s 0.551s 0.556s
lll 0.568s 0.569s 0.593s
prealloc 0.569s 0.564s 0.561s
ineffassign 0.571s 0.575s 0.585s
dupl 0.585s 0.589s 0.612s
gofmt 0.597s 0.582s 0.593s
misspell 0.606s 0.595s 0.612s
typecheck 0.653s 0.702s 0.656s
unconvert 0.658s 0.675s 0.696s
deadcode 0.663s 0.670s 0.678s
structcheck 0.670s 0.670s 0.660s
golint 0.674s 0.647s 0.671s
maligned 0.676s 0.703s 0.701s
errcheck 0.677s 0.686s 0.668s
gocritic 0.680s 0.680s 0.677s
govet 0.680s 0.659s 0.687s
depguard 0.687s 0.666s 0.693s
goimports 0.923s 0.916s 0.942s
gosec 1.016s 1.014s 1.070s
varcheck 2.999s 0.663s 0.726s

Note: I ran the test three (3) times to get an average.

All the linters are executed concurrently allowing the plugin to provide a report in less than a second, or less if the repository is smaller. The only two exceptions are “gosec” and “varcheck” which take 1-2 seconds to bootstrap their corresponding cache, but subsequent calls take significantly less time.

I’ll take your suggestion and base my improvements on SublimeLinter-stylelint.

@braver
Copy link
Member

braver commented Mar 7, 2019

Well on my laptop ESLint also takes forever to run. Luckily users can tweak how each linter runs now 😄

@cixtor I already transferred the repo, totally cool if you want to iterate on it more. Personally, I'm not going to test a Go linter, so if you can confirm for me that it works at all I'm ok with publishing it. Then, you can open a PR on the plugin repo to continue the refactor and get feedback there.

@cixtor
Copy link
Member Author

cixtor commented Mar 7, 2019

Hello @braver thank you for transferring the project to the SublimeLinter organization, hopefully this will give people more confidence when installing the plugin 🙂 I will keep maintaining it for the foreseeable future as I write Go in a daily basis, and make use of both SublimeText and SublimeLinter all the time.

Here is a GIF proving that the plugin works:

golangcilint

@braver
Copy link
Member

braver commented Mar 7, 2019

@cixtor Could you perhaps update the repo url before I merge this?

@cixtor
Copy link
Member Author

cixtor commented Mar 7, 2019

@braver done! I also moved the changes into org.json

@braver
Copy link
Member

braver commented Mar 8, 2019

Thanks. I see the alphabetical order is messed up already, I’ll clean that up later.

@braver braver merged commit 4c1a371 into SublimeLinter:master Mar 8, 2019
@kaste
Copy link
Contributor

kaste commented Mar 8, 2019

@cixtor Could you also transfer your super-computer to the org 😁 ? 😒

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

Successfully merging this pull request may close these issues.

4 participants