Simplify the 'GolangCILint' class

[nfnt]

When using a recent version of golangci-lint (I tested with v1.19.1), the generated output is
predictable enough to be parsed by a regex instead of converting JSON output.
This simplifies the code needed for linting considerably and removes the need of
a temporary file. Furthermore, by using the filename capture group introduced
in SublimeLinter 4.13, a lot of false positives are fixed.

[cixtor]

Good job! What sour people who have an older version of golangci-lint?

[kaste]

👀 You don't get that very often.

[cixtor]

I’ll run some tests during the day.

I think @nfnt did a very good job at simplifying the code, but I’m a worried this will break the functionality for people who haven’t upgraded to the latest version of golangci-lint.

Version 1.19.1 was released just yesterday that’s not enough time to let people upgrade, and that makes me hesitant to merge these changes soon; @nfnt when you tested your code with older versions of golangci-lint did you see any error?

[nfnt]

Let me check with older versions. I was on 1.18 when doing the first tests and this worked as well. But it would be great to have a good guideline on which versions work and which don't. I'll run some more tests.

[nfnt]

Just tested with v1.9 which is from July 2018 and didn't have any issues. I'm assuming that versions newer than that will work as well.

[cixtor]

Am I reading the release page wrong? https://github.com/golangci/golangci-lint/releases I can see that version 1.9.0 was released four days ago. Where did you get July 2018 from? As a reference, I am still using 1.17.1 which was released on June.

Anyway, regardless of the correctness of the code with respect to the latest version of golangci-lint available, we should at least offer a fallback in case people are using an older version.

Let’s plan something that works for you and also avoids complaints from the community.

Edit: wait, I just re-read your comment, you said 1.9 but I thought you were talking about 1.19; I apologize. If your test is correct then I guess we can release your changes soon, let me take another look and if everything is okay I’ll merge and release a new version of the plugin with your contributions.

[nfnt]

This is the v1.9 I'm referring to: https://github.com/golangci/golangci-lint/releases/tag/v1.9

[cixtor]

@nfnt here’s a few things I noticed:

1. It seems to work with Go Modules which is good.
2. With lint_mode: background the linter runs every time a modification is made in the buffer but fails to provide any useful information about errors caused by these modifications unless the user saves the changes, technically turning “background linting” into an alias for lint_mode: load_save.
3. The default output format for golangci-lint is colored-line-number which prints two additional lines for every warning/error, lines that are useless for the final report and since your proposal is using RegExp this means the linter will waste 66% of its time trying to process invalid lines. Adding --out-format tab to the command may solve this problem.
4. I am not sure how useful would it be to report the name of the tool that is reporting each error/warning. Right now all messages are associated to “golangcilint” but the golangci-lint is reporting the name of the linters in the second last column of the output. See <linter> in the example RegExp below.

^(?P<filename>[^:]+):(?P<line>\d+):(?P<col>\d+)\s+(?P<linter>\S+)\s+(?P<message>.+)$

I personally have an aversion for regular expressions, but overall your code works well and is definitely simpler than the previous one. I hope we can address at least the second point in my comment (background linting not working) and then we will be ready to release a new version.

[nfnt]

Good point regarding the output format. The regex is easy to adapt to --out-format tab. Didn't know about the <linter> capture group. I think it's a great addition to report the linter tool using this capture group. I'll look into background linting.

[kaste]

Fwiw the group has the name 'code' not 'linter'. The linter name is not under control of plugins but the backend.

[nfnt]

I changed the output format to tab and added the <code> capture group. This provides a nice linting output :)
I also marked the linter as file-only by setting tempfile_suffix = '-'.

[cixtor]

Good job @nfnt on converting the command and regex.

Regarding the addition of tempfile_suffix I’m afraid this may not be a good idea. I tried opening different projects using Go Modules and the linter immediately started showing errors about missing types that are defined in other files. This, I believe, is because the tempfile_suffix attribute instructs SublimeLinter to copy the content of the current buffer into a different location than the project where the other Go files (aka. dependencies) are located.

Here is an example where you can see this problem: https://cixtor.com/s3/example.zip

[nfnt]

Interesting, all my projects are using Go modules and I'm not seeing these issues. I'll take a look.

[nfnt]

IIUC, by setting tempfile_suffix to - nothing is copied to a temp directory. On the contrary, this is marking the linter as one that wouldn't work with temp directories so it's preventing any copies to temp directories.

[cixtor]

That’s strange. Why am I seeing this warning? 🤔

Can you please try the following:

1. Create an empty directory example
2. Initialize a Go Module: go mod init example
3. Create main.go and add this: https://play.golang.org/p/qOdDaQQN05C
4. Create types.go and add this: https://play.golang.org/p/HhZ8nKK4d4U
5. Open the entire folder in SublimeText: subl -n example
6. Check if there are any warnings/errors with Input{}

[nfnt]

Very interesting! I'm seeing a warning here as well. Don't see this warning when running golangci-lint from the CLI. And the warning is gone if I remove tempfile_suffix. Maybe tempfile_suffix isn't fully compatible with multi-file linters yet. Let me remove it.

[kaste]

@nfnt You actually just run a different command if tempfile_suffix is set.

Likewise

golangci-lint run --fast --out-format tab <filename>
golangci-lint run --fast --out-format tab 

Note that the latter opens stdin and sends the buffer content to the process/linter. This is not okay if the linter does not read from stdin as well bc it will result in BrokenPipe errors as soon as the (OS specific) buffer of stdin is full, t.i. for large files.

One solution is to set the tempfile_suffix operation mode but then add e.g. ${file_path} to the cmd or whatever suits. But the plugin actually supported background mode before. Didn't that work out well? Too slow?

[nfnt]

golangci-lint doesn't read from stdin. It also works best on multiple files, as providing a <filename> might result in false positives due to how Go packages work. Hence, this linter should be marked as file-only but still work with multiple files.

[kaste]

Last paragraph in my previous comment ☝️

[nfnt]

I'm not sure, @cixtor can probably answer this more clearly. It looks like some of the code that I'm removing was there to add background mode support for this file-only linter.

[cixtor]

But the plugin actually supported background mode before. Didn't that work out well? Too slow?

Currently the linter supports background lint mode, however, it does it in a non-performant way —by copying the content of the project into a temporary directory— because the content of the buffer needs to be in the same directory as the other files, otherwise golangci-lint will print irrelevant warnings/errors, as @nfnt explained.

Slowness is not really a problem, golangci-lint has a global timeout of one second. All the integrated linters will be closed before that, and hopefully the --fast flag has a good selection of fast linters so force-closing may not be common practice for them. So the plugin runs fast and there are some benchmarks in the code to prove it.

However, the README explains a limitation in the current implementation. To work in background mode, the linter needs to copy the project to a different folder, but many Go projects are considerably large. If the linter aborts the operation if it detects more than settings.delay * 1000 files in the project.

The linter will also abort the operation if it detects the project is using a canonical import path because in that case copying the content of the project to a temporary folder will immediately fail because the file path is different.

And after saying that I realized that the current implementation doesn’t supports Go Modules in “background” lint mode because it is not copying the go.mod and go.sum files to the temporary folder.

---

Anyway, the point is, the current implementation kind of supports background lint mode but only in relatively small projects. But even if we manage to fix that, there are other limitations imposed by Go itself, canonical import paths are just one of them. The only way this plugin works well is when lint mode is set to load_save or any other value that requires the user to save the buffer before the linter runs. I believe the majority of Go programmers are okay with this considering the most popular tools in the ecosystem also work the same way.

All of this is to say that the current code, while not perfect, has already been iterated several times to handle a handful of annoying edge cases. While the code proposed by @nfnt is interesting and clever, it takes us back to the very beginning where the edge cases are not handled and we have to start all over again testing and fixing bug reports.

[kaste]

Hm, as I understand the code, we linted a directory and not a project (t.i. a folder recursively). Hard to believe someone has more than 1000 files in one directory but okay.

The linter website says if we omit the last args it defaults to ./.... So we basically have to decide recursively or not.

tempfile_suffix = '-'  # for the operation mode, saved files only
cmd = 'golangci-lint run --fast ${file_path}' # OR
cmd = 'golangci-lint run --fast ${file_path}/...'  # OR
cmd = 'golangci-lint run --fast ${folder}/...'  # project top folder
cmd = 'golangci-lint run --fast ${xoo:./...}'  # working dir (can be set by the user)

Not recursively should be faster, so if it returns sane results I would go for that. But if it's not reliable enough, I wouldn't confuse users and pick the slower one.

Should this be configurable? Is it plausible that per project someone wants to include a specific folder always, e.g. ... $file_path ./other_dir /always/important?

[nfnt]

Thanks for the detailed explanations, I wasn't really aware how background-mode works under the hood. Did some test with enabled debug-logging and found a (IMO) good solution for file-only linting. Due to the way packages are working on Go, every file in file_path needs to be linted. This way no package-relevant informations are missed by the linters and false positives (like the one described above) are avoided. Hence, I changed cmd to golangci-lint run --fast ${file_path}.

[cixtor]

That sounds good. Can you add a visible warning for users with lint_mode: background? Considering the plugin will not work in that scenario, the least we can do is to show a message suggesting users to use lint_mode: load_save instead.

[kaste]

SublimeLinter will choose the best mode applicable if background is set. Users don't have to change their settings for that. But sure a note that the plugin now only works on saved files bc they will notice that and wonder.

[nfnt]

Good point! Updated the README accordingly.

[cixtor]

Sorry for the long delay, I just checked and it seems to work okay. Although I was expecting an in-editor warning rather than a message in the README, but I guess people who are using this plugin are smart enough to look for answers in the repository if they notice the plugin is not doing anything, eventually they will find the warning in the README.

---

Is this something we should take care of in this pull-request?

SublimeLinter: #1 linter.py:769       WARNING: golangcilint output:
level=warning msg="[runner] Can't run linter goanalysis_metalinter: S1002:
  failed prerequisites:
    inspect@pkg.cixtor.com/<PRIVATE>/datalayer,
    isgenerated@pkg.cixtor.com/<PRIVATE>/datalayer"

SublimeLinter: #1 linter.py:771       Note: above warning will become an error
  in the future. Implement `on_stderr` if you think this is wrong.

Maybe not since this is probably due to the nature of the Go packages that I am hosting privately. It would be nice to have a visible warning about this. I only found the warning after inspecting the SublimeText console. I think I will add something later.

I’ll test a bit more and merge then release in a few hours.

[braver]

Nice work guys! 🎉