mfussenegger / nvim-lint

An asynchronous linter plugin for Neovim complementary to the built-in Language Server Protocol support.
GNU General Public License v3.0
1.94k stars 204 forks source link

feat: add default severity option #562

Closed doums closed 6 months ago

doums commented 6 months ago

fix #557

Allows to customize the default diagnostic severity used for certain linters

mfussenegger commented 6 months ago

Thanks, but I'm not going to add this

doums commented 6 months ago

@mfussenegger then can you give a little more detail on why you reject the PR? Or how would you like to see the feature implement perhaps?

mfussenegger commented 6 months ago

Couple of reasons:

doums commented 6 months ago

I haven't heard a compelling explanation why this should be configurable Or why the current default choices in the few changed linters are bad

I thought #557 make it clear enough, but seems not. So there is my explanations, for my specific case. I'm using cspell to spot spelling issues in my code. But cspell can produce a lot of diagnostics (including its batch of false positives), making code hard to read. So I want it to produce its diagnostics at the minimum severity → HINT level. For that purpose, I configured my neovim to report all hint diags as discreetly as possible (no sign, no virt text, just underline dashed using low contrast color).

Or why the current bad defaults can't be simply changed

Setting the defaults to different other arbitrary values will result with the same issue: for some users it will work, for other it will not, it depends on what user need for their specific use case. So why not allow customization of this default?

It's incomplete. Next up I'd be seeing issues about the changed default not being applied to linter XY

On purpose, I only changed linters which have only one level of severity. For those which can produce different levels diags, there is no need to change the severity mapping.

mfussenegger commented 6 months ago

For that purpose, I configured my neovim to report all hint diags as discreetly as possible (no sign, no virt text, just underline dashed using low contrast color).

Isn't vim.diagnostic.config(opts, require("lint").get_namespace("cspell")) good enough to turn off signs and virt text for cspell?

For those which can produce different levels diags, there is no need to change the severity mapping.

I don't really see the difference. If you disagree with the choice of defaults, why'd that be limited to the static cases? There have been cases where I ended up merging changes to the severity based on the diagnostic code (https://github.com/mfussenegger/nvim-lint/commit/06e4bb431f3408b75466ede4234490ae7fcf17cb)

Another limiting aspect of the proposed change here is that it would affect all (selected) linters the same way. What if you want to have editorconfig-checker on warning, but cspell on hint level?

I'd rather add something like a wrap function in lint.utils that allows to map the diagnostic entries and override a linter somewhat like this:

local lint = require("lint")
lint.linters.cspell = lint.util.wrap(lint.linters.cspell, {}, function(diagnostic)
  diagnostic.severity = vim.diagnostic.severity.HINT
  return diagnostic
end)

Note that this kind of wrapping is already possible, the util function would only make it a bit more convenient.

doums commented 6 months ago

Isn't vim.diagnostic.config(opts, require("lint").get_namespace("cspell")) good enough to turn off signs and virt text for cspell?

Yes but it does not allow customizing the highlight group, for instance I'd need the hint underline color for cspell

Another limiting aspect of the proposed change here is that it would affect all (selected) linters the same way. What if you want to have editorconfig-checker on warning, but cspell on hint level?

Yes good point. I initially worked on to support that, but as there is no setup function exposed by the plugin, it's limiting the way for more proper configuration customization and config checking logic. So I ended up to keep it really simple

I'd rather add something like a wrap function in lint.utils that allows to map the diagnostic entries and override a linter somewhat like this

It will do the trick, seems a good solution

mfussenegger commented 6 months ago

Could you check https://github.com/mfussenegger/nvim-lint/pull/564 ?