Skip to content
This repository has been archived by the owner on Aug 12, 2023. It is now read-only.

Add golangci-lint for golang linting #318

Closed
wants to merge 1 commit into from

Conversation

zalegrala
Copy link

No description provided.

@zalegrala zalegrala force-pushed the golangci branch 2 times, most recently from dd65744 to 37ba7ec Compare November 5, 2021 21:54
@zalegrala
Copy link
Author

I can't tell what the linter is complaining about. Any clues?

@jose-elias-alvarez
Copy link
Owner

Thank you for the PR! I think the linter has an issue with the number of spaces? I checked out the PR locally and stylua reported the same issue, so I think you just have to run the file through stylua.

Also: We had a previous PR that included this linter, and we've yet to merge it because of the issues discussed there. Could you verify whether your source updates diagnostics on change before saving the file? If not, I think it might be better to wait until I (or someone else) gets around to working on #166.

@zalegrala
Copy link
Author

The diagnostics show up before save. If I open a new file I see the diagnostics as soon as golangci-lint can run. I've pushed an update after running with stylua.

@zalegrala
Copy link
Author

The linter is now failing for an unrelated change. Do you want me to address here as well?

@jose-elias-alvarez
Copy link
Owner

Apologies – I should have been more specific. The issue isn't whether diagnostics are shown when first opening a file but rather whether diagnostics are updated when making a change to a file (before writing it to the disk). I briefly tested out and found that this PR suffers from the same issue.

I'm not opposed to adding support for this class of linters in the future but am not sure about merging this right now, mostly since the inconsistency in observed behavior may lead users to conclude that something is broken. As I mentioned in the linked issue, it seems that the golangci-lint extension for VS Code only runs on save, so I think that's an acceptable compromise once we've gotten that implemented.

@zalegrala
Copy link
Author

Oh I see what you're saying. If the buffer gets updated, I don't think the changes are reflected, since the execution is performed based on what is on disk. I suppose I didn't notice since that's how the tool operates. I do appreciate when the linters can be updated before saving, but as mentioned in the linked issues, this seems to be a tool limitation for this linter. I was hopeful that this would be the fast path to being able to use golangci-lint for my config, but if I need to come up with another solution in the short term that's not a problem. Let me know if I can do anything else to help this along.

@jose-elias-alvarez
Copy link
Owner

Sure, thanks. In the meantime, if you're happy with the current functionality you can register it as a custom source:

local null_ls = require("null-ls")

local golangci_lint = {
    method = null_ls.methods.DIAGNOSTICS,
    filetypes = { "go" },
    generator = null_ls.generator({
        command = "golangci-lint",
        to_stdin = true,
        from_stderr = false,
        args = {
            "run",
            "--out-format",
            "json",
            "$DIRNAME",
            "--path-prefix",
            "$ROOT",
        },
        format = "json",
        check_exit_code = function(code)
            return code <= 2
        end,
        on_output = function(params)
            local diags = {}
            for _, d in ipairs(params.output.Issues) do
                if d.Pos.Filename == params.bufname then
                    table.insert(diags, {
                        row = d.Pos.Line,
                        col = d.Pos.Column,
                        message = d.Text,
                    })
                end
            end
            return diags
        end,
    }),
}
-- add to other sources or register individually
null_ls.register(golangci_lint)

@zalegrala
Copy link
Author

Oh cool, thanks for that.

@kylo252
Copy link
Contributor

kylo252 commented Nov 9, 2021

I was checking the docs for this at https://golangci-lint.run/usage/integrations:

Using it in an editor without --fast can freeze your editor.

So maybe you should add it. You can also see all the flags that are used by flycheck as a reference.

@zalegrala
Copy link
Author

I've added --fast though that appears to be guidance for vscode. Should we close this out? I see that its already out of date with what is in main.

@jose-elias-alvarez
Copy link
Owner

It's up to you – to resolve conflicts you'd want to move the source into its own file, which is the structure we're using since d78734b. Once we support running on textDocument/didSave, you'd just have to change the method and this would be ready to merge.

@zalegrala
Copy link
Author

Ah okay thank you, just a new structure. I've updated to accommodate.

Copy link
Contributor

@marcomayer marcomayer left a comment

Choose a reason for hiding this comment

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

Would love to finally see golangci-lint in null-ls, here are some suggestions.

generator_opts = {
command = "golangci-lint",
to_stdin = true,
from_stderr = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding ignore_stderr = true, might be a good idea to not have linter deprecation warnings in the console (to test use --enable-all as an arg to golangci-lint.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I'd added suppress_errors = true in my local version which seemed to suppress the deprecation. Is ignore_stderr preferred? The warnings all seem to go to stderr, while the json goes to stdout.

Copy link
Author

Choose a reason for hiding this comment

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

I see too that there is some json to collect warnings, but not sure how to represent that in the linter.

...
"Report": {
    "Warnings": [
      {
        "Tag": "runner",
        "Text": "The linter 'golint' is deprecated (since v1.41.0) due to: The repository of the linter has been archived by the owner.  Replaced by revive."
      },
      {
        "Tag": "runner",
        "Text": "The linter 'interfacer' is deprecated (since v1.38.0) due to: The repository of the linter has been archived by the owner. "
      }
    ],

Copy link
Contributor

Choose a reason for hiding this comment

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

suppress_errors got deprecated and completely replace with ignore_stderr. Not sure about the deprecated warnings...from what I know it's not so clear how to properly deal with those even from a golangci-lint perspective. I guess in some new version they'll be removed so I just ignore the warnings for now...

Copy link
Author

Choose a reason for hiding this comment

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

Updating the configuration fixes the issue, I was mostly just pointing out that the information that is sent to stderr also seems to be present in the json. So if there were a preferred way to access that from vim, then maybe we could do it. I too mostly ignore them too, but perhaps I shouldn't, or perhaps users might want to know somehow. I was slightly wondering if there was a way to get those into a quickfix list or something.

Choose a reason for hiding this comment

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

We don't currently do anything with warnings like this unless they prevent the linter from running in the first place, so I think it's safe to ignore them. @marcomayer Are you able to test out #339?

Copy link
Contributor

@marcomayer marcomayer Nov 12, 2021

Choose a reason for hiding this comment

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

@jose-elias-alvarez Great so you really want to add this to #339, also the default severity. I'm happy to test-drive this next week at work. For now, I can already say that the diagnostics do show up when saving a file but disappear as soon as I edit anything. I'm on nvim 0.5.1. That's at least unexpected and at least for me would be not something I'd like to work with :/

table.insert(diags, {
row = d.Pos.Line,
col = d.Pos.Column,
message = d.Text,
Copy link
Contributor

Choose a reason for hiding this comment

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

By default, all issues are "errors" which I think is a bit too hard. Not the perfect solution but maybe better, for now, is to default to warnings:
severity = h.diagnostics.severities["warning"],
For this to work you need local h = require("null-ls.helpers")

Copy link
Author

Choose a reason for hiding this comment

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

I'd seen some usage of severity in other linters, but wasn't clear on its use. What is that doing?

Copy link
Contributor

Choose a reason for hiding this comment

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

When running the null-ls linter you'll have the results shown as diagnostics in nvim. There are different error levels aka serenity-levels there. By default they're seen as errors which is not true for most of the linters. An error would be something that really breaks the code like usage of a non-existing variable or a syntax error. But something like "this line is too long" or "you should add a comment here" doesn't break the code so you don't want it popping up as an error but as a warning. So adding severity explicitly changes the default from error to warning in this case.

I think you can set severity per golangci-linter config and it would then actually show up in the json results (by default it's empty). So ideally you'd check if there's something in there and only set the warning default in case it's not.

@zalegrala
Copy link
Author

Fixed in #339

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

Successfully merging this pull request may close these issues.

4 participants