ale

ale copied to clipboard

Hi @w0rp,

Here is a PR for the changes we discussed in #4005.

Changes

• g:ale_send_to_neovim_diagnostics = 0 | 1 as a configuration option
• When g:ale_send_to_neovim_diagnostics is enabled, we send ALE output to Neovim diagnostics
• A version check for Neovim 0.6+. We print an error if the config is set to 1 and the user is on an unsupported Vim version
• Documentation

TODOs

• [ ] Fix off by 1 issue reported by @daliusd below
• [ ] Maybe good idea to add a test test_linting_sets_signs.vader or test_sign_placement.vader that tests ALE signs are disabled when neovim diagnostics are enabled.
• [ ] Also maybe add a test to test_cursor_warnings.vader that tests cursor messages is not echoed when neovim diagnostics are enabled.
• [ ] Figure out if it's OK to disable loclist/quickfix like we do currently
• [ ] Fix issue with g:ale_virtualtext_cursor reported below

Screenshots

Error on startup if you're not running Neovim 0.6+

ALE in the diagnostics API

ALE in trouble.nvim's fix list

Testing

Here's an example of ALE now unified with nvim-lsp's diagnostics (in this case Sorbet) inside of trouble.nvim:

@bluz71 Since you were the original requester for this, would you be down to test this branch out on your end as well, and see if this config works for you?

Cool stuff :) I actually never used ALE before, and started with nvim's LSP and diagnostics. But some old plugins still not supporting nvim's diagnostics, but do support ALE. And I was missing some diagnostic's features, like virtual text (partially support with ale_virtualtext_cursor, but not exactly what I was used to).

So I decide to switch to your branch :)

Here is first thing that jumps into my eyesight:

Duplication happens if ale_virtualtext_cursor is on.

I think it behaves like because of auto command

https://github.com/dense-analysis/ale/blob/d3df00b89803f1891a772c47fc8eda6a1e9e1baa/autoload/ale/events.vim#L142-L148

@ajukraine Is this because diagnostics and ALE are both using virtualtext?

Is the right solution simply to call this out in the docs as something you might want to disable with g:ale_virtualtext_cursor = 0?

@dbalatero yes, they both use virtual text.

But the problem is even a bit deeper. your current implementation takes care only of ale#engine#SetResults part of the logic. However virtual text feature is also used in events.vim. Which means that after your change the behaviour is not consistent. It also applies to "echo cursor" functionality.

So I think there at least 3 ways of moving forward:

• Let ALE's users decide whether to disable ALE's virtual text functionality or not; Is OK for power users, but will be confusing for beginners
• Make the current change consistent and follow the same logic everywhere around ALE's code base (from what I can see it should be at least repeated in events.vim; maybe somewhere else, needs to be investigated)
• Make the current change consistent, but move the logic into deeper levels of code base (virtualtext.vim and cursor.vim)

So I think there at least 3 ways of moving forward:

Makes sense! I think I'd defer to @w0rp for preferences here, but I'm happy to follow any of these paths!

@bluz71 Since you were the original requester for this, would you be down to test this branch out on your end as well, and see if this config works for you?

Apologies, I am not in a position to test this since I did switch over to null-ls about 4 or so months back.

Thankfully there are other testers available.

Cheers.

Thanks for this PR!! This is a killer feature for me.

I am still using ALE due to these reasons for phpcs and phpstan. I was maintaining a whole separate UI for ALE errors in virtual text, floating windows, sign column, tabs, keymaps, etc. With your PR I can get rid of all that hacky code.

Hopefully I can eventually switch these last few tools over to null-ls too, but not until these problems are fixed.

@w0rp any feedback so far on this PR? I can address any items that jump out to you. Also curious about your thoughts on @ajukraine's comments!

Actually on further testing it seems that the cursor echo and virtual text are still enabled even with neovim diagnostics enabled:

The red "E" and virtual text I believe are rendered by neovim diagnostics. The virtual text and echo at the bottom of the screenshot are rendered by ALE.

I do not use neovim diagnostics but from this screenshot I belive it handles "Signs" and "Virtual Text"? If this is the case then I would only disable those two in ALE and leave everything else untouched (e.g. echo cursor, loclist/quicklist, highlights, etc).

I have tried this and it is great. Please rebase from master as master has important fixes.

@hsanson Thanks for looking at the PR! Would it be possible to add Neovim 0.6+ to the test matrix in CI? If so I could rebase this branch on top of that change and add more tests in that specifically use the new 0.6 APIs that this PR is based off of.

I'm sure this is good. I'll have a look through soon when I can.

@daliusd I have rebased this branch on master again, so it should be up to date as of now.

Thanks @w0rp. Curious what your thoughts are about the issue reported by @ajukraine!

@hsanson Thanks for looking at the PR! Would it be possible to add Neovim 0.6+ to the test matrix in CI? If so I could rebase this branch on top of that change and add more tests in that specifically use the new 0.6 APIs that this PR is based off of.

Latest master already tests againt neovim 0.6.1 and this new branch #4180 that needs review tests against 0.7.

@dbalatero So it's easy to understand, what I try to do with the tests is run them in CI with:

1. The oldest Vim version supported.
2. The oldest NeoVim version supported.
3. The latest Vim version.
4. The latest NeoVim version.

I try not to ever drop support for any versions at will, but instead very carefully. I'm waiting for the day when NeoVim hits 1.0. I actually won't use NeoVim myself until it does. I have become... a 30 year old boomer. I run extremely bog standard Linux software these days and rarely use anything that isn't completely stable. I'm just very conscious of how I spend my time these days. :smiley:

Thanks for the review @w0rp! Appreciate the approval but it sounds like there are changes I need to make, so you might want to just mark it as Pending changes/comments until I've addressed everything.

@bluz71 Since you were the original requester for this, would you be down to test this branch out on your end as well, and see if this config works for you?

@dbalatero,

I have had time to have a look at this impressive work.

Well done; especially since I am mulling switching back to ALE from null-ls (due to some quirks of null-ls).

Though I have encountered a bug with with the mdl Markdown linter and this PR.

My rough ALE + Neovim diagnostics setup:

local g = vim.g

g.ale_linters = {
  markdown = {'mdl'},
}

g.ale_completion_enabled = 0
g.ale_echo_cursor = 0
g.ale_hover_cursor = 0
g.ale_lint_on_enter = 0
g.ale_lint_on_filetype_changed = 0
g.ale_lint_on_insert_leave = 0
g.ale_lint_on_save = 1
g.ale_lint_on_text_changed = 'never'
g.ale_linters_explicit = 1
g.ale_send_to_neovim_diagnostics = 1

local map = vim.keymap.set
map("n", "]d", "<cmd>lua vim.diagnostic.goto_next()<CR>")
map("n", "[d", "<cmd>lua vim.diagnostic.goto_prev()<CR>")

Load up a Markdown files such as this one and delete the black line at line 3, this should now produce a lint warning (correctly).

Now hit ]d to go to the next warning. That produces this failure for me:

I am using Neovim 0.7.0 (official release).

My quick testing with Standard Ruby and Standard JS linters (via this PR) does not produce the same issue.

So linter specific.

@dbalatero hey, how are you doing? I am using your branch for a month and its awesome. Do you need helping hand with fixing minor issues?

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests. If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

☹️

Can this be re-opened?

@daliusd I'm still interested in getting this merged. Sorry to everyone for not being around. If you can make the adjustments I left in my review back in May, we can merge this into master. There wasn't all that much I wanted to change. I just haven't had the time to adjust it myself and test it in NeoVim. You might have a better idea as a regular NeoVim user. (I'm still using Vim until I am pretty sure NeoVim is tagged as 1.0.0 and there's a semi-official GUI client.)

@daliusd I'm still interested in getting this merged. Sorry to everyone for not being around. If you can make the adjustments I left in my review back in May, we can merge this into master. There wasn't all that much I wanted to change. I just haven't had the time to adjust it myself and test it in NeoVim. You might have a better idea as a regular NeoVim user. (I'm still using Vim until I am pretty sure NeoVim is tagged as 1.0.0 and there's a semi-official GUI client.)

@w0rp sorry, I have migrated to null-ls.nvim. I can't help with this.

I'll try and take this over…I'm interested in it enough at least.

See #4345 for my resurrection of this PR. Thanks for the base @daliusd!

Hey @mathstuf thanks for taking this over–life took over on my end and I lost the bandwidth to get this one done. Should I close this PR in lieu of yours?

Yeah, I think that'd be fine. I wish there was a way to get those Cc'd here Cc'd on a superceding issue/PR, but that's a long-standing feature request from my end here anyways :) .