refactor(diagnostic)!: replace 'show_*' functions with 'open_float' #16057

gpanders · 2021-10-17T16:24:05Z

show_line_diagnostics() and show_position_diagnostics() are almost identical; they differ only in the fact that the latter also accepts a column to form a full position, rather than just a line. This is not enough to justify two separate interfaces for this common functionality.

Renaming this to simply show_diagnostics() is one step forward, but that is also not a good name as the _diagnostics() suffix is redundant. However, we cannot name it simply show() since that function already exists with entirely different semantics.

Instead, combine these two into a single open_float() function that handles all of the cases of previewing diagnostics in a floating window. Also add a "float" key to vim.diagnostic.config() to provide global values of configuration options that can be overridden ephemerally. This makes the preview API consistent with the rest of the diagnostic API.

This is a breaking change.

Differences in API usage:

Before	After
`vim.diagnostic.show_line_diagnostics()`	`vim.diagnostic.open_float(0, {scope="line"})`
`vim.diagnostic.show_position_diagnostics()`	`vim.diagnostic.open_float(0, {scope="cursor"})`
`vim.diagnostic.goto_next { enable_popup = true, popup_opts = {...}`	`vim.diagnostic.goto_next { float = {...} }`
`vim.diagnostic.show_line_diagnostics({opts}, buffer, 17)`	`vim.diagnostic.open_float(buffer, {{opts}, scope="line", pos=17}`
`vim.diagnostic.show_position_diagnositcs({opts}, buffer, {row, col})`	`vim.diagnostic.open_float(buffer, {{opts}, scope="cursor", pos={row, col}}`

clason · 2021-10-17T16:27:24Z

This is a breaking change.

Worth it :) (Especially with that handy migration table ❤️ )

clason · 2021-10-17T16:31:08Z

Not perfectly sold on the where keyword, though -- that could be mistaken for the location of the float (although that would properly be set in the options table)?

But the alternatives I can come up with (at, location) have the same problem...

EDIT from, maybe?

gpanders · 2021-10-17T16:33:06Z

Not perfectly sold on the where keyword, though -- that could be mistaken for the location of the float (although that would properly be set in the options table)?

But the alternatives I can come up with (at, location) have the same problem...

EDIT from, maybe?

Maybe scope?

mjlbach · 2021-10-17T18:35:40Z

I also prefer float over popup where possible, and would favor an enum over string based scope. Also, I kinda like show more than preview, as preview implies a slice of diagnostics, which you could argue is a slice of the diagnostic cache, but I like the semantics of show more and would also favor reworking other usages of it.

If we're doing this breaking change, I would also favor at the same time handling the deprecations of all of the lsp.diagnostics holdover, so we avoid multiple breaking changes, and merging this around the 0.6 feature freeze.

mjlbach · 2021-10-17T18:50:32Z

Also we should fix the flakey win32 test (unrelated to this PR, just noting). Gradually these windows tests are becoming more and more reliable...

2021-10-17T16:39:27.9031288Z   [  FAILED  ] test/functional\core\job_spec.lua @ 877: jobs jobstop() kills entire process tree #6530
2021-10-17T16:39:27.9032173Z   test/functional\core\job_spec.lua:949: retry() attempts: 428
2021-10-17T16:39:27.9033006Z   test\helpers.lua:73: Expected objects to be the same.
2021-10-17T16:39:27.9063321Z   Passed in:
2021-10-17T16:39:27.9063978Z   (boolean) false
2021-10-17T16:39:27.9064556Z   Expected:
2021-10-17T16:39:27.9065103Z   (boolean) true

kylo252 · 2021-10-17T19:51:43Z

This is looking great already ❤️

I'm still not sure about the function name as I feel it can suggest that there's a more detailed diagnostics view, when in most cases the "preview" is all you're gonna get anyway. I think it might be worth it to (re-)consider vim.diagnostic.show_hover() instead.

gpanders · 2021-10-17T20:09:32Z

I also prefer float over popup where possible

I don't think there are any (user-visible) instances of "popup" anywhere (except for the return type of preview()), please correct me if I'm wrong.

and would favor an enum over string based scope.

I would too, but with module scopes an enum quickly becomes unwieldy: vim.diagnostic.preview.BUFFER vs "buffer". That's why I stuck with a string. Maybe that's fine though since this function won't ever be called interactively.

Also, I kinda like show more than preview, as preview implies a slice of diagnostics, which you could argue is a slice of the diagnostic cache, but I like the semantics of show more and would also favor reworking other usages of it.

IMO show() and hide() are good names for what they do right now; namely, showing persistent decorations for diagnostics (virt text, signs, highlights, etc.). preview() is a "preview" in that it only shows ephemerally, it does not persist in the buffer in any way. Besides, the function that preview() calls is itself called open_floating_preview(), so there's precedent.

If we're doing this breaking change, I would also favor at the same time handling the deprecations of all of the lsp.diagnostics holdover, so we avoid multiple breaking changes, and merging this around the 0.6 feature freeze.

Are you suggesting hard-deprecating lsp.diagnostics by removing those functions completely? I'm not sure I agree with that approach, I think I'd favor introducing vim.diagnostic and soft-deprecating lsp.diagnostics (we can show an info message when these functions are used warning that they are deprecated. vim-fugitive did this IIRC) in 0.6 and then hard-deprecating (removing) them in the next release.

mjlbach · 2021-10-17T20:35:53Z

I don't think there are any (user-visible) instances of "popup" anywhere (except for the return type of preview()), please correct me if I'm wrong.

Referring to the internal usage, but I see you changed it.

Are you suggesting hard-deprecating lsp.diagnostics by removing those functions completely? I'm not sure I agree with that approach, I think I'd favor introducing vim.diagnostic and soft-deprecating lsp.diagnostics (we can show an info message when these functions are used warning that they are deprecated. vim-fugitive did this IIRC) in 0.6 and then hard-deprecating (removing) them in the next release.

I'm suggesting a soft deprecation with printed warnings and removing the compatibility layer during the stabilization period leading up to 0.6. Additionally we should be removing the remaining internal usages of deprecated functions (there are still some)

IMO show() and hide() are good names for what they do right now; namely, showing persistent decorations for diagnostics (virt text, signs, highlights, etc.). preview() is a "preview" in that it only shows ephemerally, it does not persist in the buffer in any way. Besides, the function that preview() calls is itself called open_floating_preview(), so there's precedent.

Fair, I still like display/hide and show() from the old style but I may be in the minority.

I would too, but with module scopes an enum quickly becomes unwieldy: vim.diagnostic.preview.BUFFER vs "buffer". That's why I stuck with a string. Maybe that's fine though since this function won't ever be called interactively.

I was thinking like vim.diagnostic.location.BUFFER with validation the location type is in the enum.

justinmk · 2021-10-17T20:49:21Z

IMO show() and hide() are good names for what they do right now; namely, showing persistent decorations for diagnostics (virt text, signs, highlights, etc.). preview() is a "preview" in that it only shows ephemerally, it does not persist in the buffer in any way. Besides, the function that preview() calls is itself called open_floating_preview(), so there's precedent.

'previewwindow' has long-established semantics in Vim already. I wonder where open_floating_preview came from, its doc is Shows contents in a floating window.. That's literally what nvim_open_win does, but "preview" was thrown in the name... it could be named show_float or open_float. Please don't perpetuate "preview" because of this "precedent".

Fair, I still like display/hide and show() from the old style but I may be in the minority.

Those are better than randomly sprinkling "preview" in function names for no reason.

In general though the :help api names should be considered first. The verb "open" is established there. If "show" is needed for some reason, it should be a conscious reason.

mjlbach · 2021-10-17T20:51:36Z

@justinmk Yeah I've never liked preview as a name, or popup becomes of the semantic association with prior vim semantics.

"preview" was thrown in the name... it could be named show_float or open_float

+1 to changing this

runtime/doc/diagnostic.txt

runtime/lua/vim/diagnostic.lua

ckipp01 · 2021-10-19T10:59:16Z

Since this is a breaking change, I wanted to throw this out there as well (as a follow-up to a convo with @clason on Gitter). I know it's not exactly what you're addressing here, but might be worth at least thinking about it since we are breaking stuff. Right now we treating severity the same all over the space at a global level with severity_sort being set to false by default. Now that float stuff is being sort of extracted out, do we still want this? I know for things like signs, I find this odd at my error signs aren't shown ahead of my warning signs since both are treated as the same level. Especially when using LSP I sort of think the expectation is that errors take precedence and should be shown ahead of warnings. I am of the mindset that having severity_sort = true is a more sane default that false. Any thoughts on this or thoughts on if this could be addressed as we are now breaking float out?

runtime/lua/vim/diagnostic.lua

justinmk

Looks like a good direction. Thanks @gpanders again for your attention to detail :)

mjlbach · 2021-10-19T14:54:13Z

+1 other than virt textual (unless you want to go even farther and make it virtual textual reality)

Right now we treating severity the same all over the space at a global level with severity_sort being set to false by default. Now that float stuff is being sort of extracted out, do we still want this? I know for things like signs, I find this odd at my error signs aren't shown ahead of my warning signs since both are treated as the same level. Especially when using LSP I sort of think the expectation is that errors take precedence and should be shown ahead of warnings. I am of the mindset that having severity_sort = true is a more sane default that false. Any thoughts on this or thoughts on if this could be addressed as we are now breaking float out?

This was me. I'm of two thoughts of this:

It's hypothetically possible for the server to control the diagnostic order (although I haven't checked if it's actually preserved)
I wanted to avoid unnecessary iteration + sort operations over tables

I'm not strongly opposed to making severity_sort the default, given it now serves more sources than lsp, but these were the original two justifications I gave for not making this the default as it was introduced.

mjlbach

virt textual but then LGTM

'show_line_diagnostics()' and 'show_position_diagnostics()' are almost identical; they differ only in the fact that the latter also accepts a column to form a full position, rather than just a line. This is not enough to justify two separate interfaces for this common functionality. Renaming this to simply 'show_diagnostics()' is one step forward, but that is also not a good name as the '_diagnostics()' suffix is redundant. However, we cannot name it simply 'show()' since that function already exists with entirely different semantics. Instead, combine these two into a single 'open_float()' function that handles all of the cases of showing diagnostics in a floating window. Also add a "float" key to 'vim.diagnostic.config()' to provide global values of configuration options that can be overriden ephemerally. This makes the float API consistent with the rest of the diagnostic API. BREAKING CHANGE

mfussenegger

Nice. Really great improvement 👍

mfussenegger · 2021-10-19T16:14:35Z

runtime/lua/vim/diagnostic.lua

+      error("Invalid value for option 'pos'")
+    end
+  elseif scope ~= "buffer" then
+    error("Invalid value for option 'scope'")


Not sure if worth it here - but including the user provided value and the valid options often help users figure out what went wrong.

gpanders · 2021-10-19T17:00:46Z

runtime/lua/vim/diagnostic.lua

+  do
+    -- Resolve options with user settings from vim.diagnostic.config
+    -- Unlike the other decoration functions (e.g. set_virtual_text, set_signs, etc.) `open_float`
+    -- does not have a dedicated table for configuration options; instead, the options are mixed in
+    -- with its `opts` table which also includes "keyword" parameters. So we create a dedicated
+    -- options table that inherits missing keys from the global configuration before resolving.
+    local t = global_diagnostic_options.float
+    local float_opts = vim.tbl_extend("keep", opts, type(t) == "table" and t or {})
+    opts = get_resolved_options({ float = float_opts }, nil, bufnr).float
+  end


Thanks to @kylo252 for pointing this out. open_float works differently than set_virtual_text and friends since it doesn't have a dedicated table for configuration options, so if we resolve opts directly that table clobbers the global configuration. As an example, consider

vim.diagnostic.config({ float = { show_header = false } }) vim.diagnostic.open_float(0, { scope = "line" })

If we pass {scope="line"} directly to get_resolved_options then it will assume that that is the full options table, which is missing show_header and therefore will default to its unset value (true) even though it's set to false in the global configuration.

One way to fix this would be to use tbl_deep_extend in get_resolved_options, which is what I tried first, but that removes the ability to use an empty table to revert to default values (#16043). Another way would be to make a nested opts table:

vim.diagnostic.open_float(0, {scope="line", opts = {...}})

But that seems confusing and non-intuitive to me. So instead just fill in missing keys from the global configuration before doing option resolution.

sQVe · 2021-10-19T18:39:18Z

@gpanders I just tried this out and one thing that no longer acts the same way is that triggering open_float twice doesn't move the cursor into the float. This was something that I thought was really useful for yanking diagnostics etc.

Is there a way to get this functionality?

gpanders · 2021-10-19T18:47:51Z

That is a regression, it's an easy fix though. I'll address that in a follow up PR.

sQVe · 2021-10-19T18:50:02Z

@gpanders Awesome 🎉 Thank you!

I also noted that show_header is supported in open_float but not in goto_next or goto_prev.

gpanders · 2021-10-19T19:03:01Z

I also noted that show_header is supported in open_float but not in goto_next or goto_prev.

It is:

vim.diagnostic.goto_next { float = { show_header = true } }

See neovim/neovim#16057

n3wborn · 2021-10-19T22:24:53Z

Damned ! Sorry, I didn't think my commit would appear here :/

- nightly neovim/neovim#16057 changed how line diagnostics are shown/rendered.

megalithic · 2021-10-26T13:34:36Z

@gpanders I've noticed that this particular bug has crept back when using open_float via autocmd; should we be handling the closing of this float on BufLeave on our own? (#12477)

gpanders · 2021-10-26T14:22:22Z

I would be surprised if this PR were the cause of that. It looks like "BufLeave" was removed from the default list of autocommands in #14649.

You can try adding

close_events = { "CursorMoved", "CursorMovedI", "BufHidden", "InsertCharPre", "BufLeave" }

to the options table you pass to open_float. If that works, feel free to send a PR adding "BufLeave" to the default list of close_events.

ref: neovim/neovim#16057

`open_float()` is replacement for `show_line_diagnostic()`, the opts still the same, but buffer number required as the first arguments, 0 for current buffer. ref: neovim/neovim#16057

Replace `enable_popup` and `popup_opts`. ref: neovim/neovim#16057

…eovim#16057) 'show_line_diagnostics()' and 'show_position_diagnostics()' are almost identical; they differ only in the fact that the latter also accepts a column to form a full position, rather than just a line. This is not enough to justify two separate interfaces for this common functionality. Renaming this to simply 'show_diagnostics()' is one step forward, but that is also not a good name as the '_diagnostics()' suffix is redundant. However, we cannot name it simply 'show()' since that function already exists with entirely different semantics. Instead, combine these two into a single 'open_float()' function that handles all of the cases of showing diagnostics in a floating window. Also add a "float" key to 'vim.diagnostic.config()' to provide global values of configuration options that can be overridden ephemerally. This makes the float API consistent with the rest of the diagnostic API. BREAKING CHANGE

see: neovim/neovim#16057

See explainations here: neovim/neovim#16057

There have been breaking changes to that API: ref neovim/neovim#16057

github-actions bot added diagnostic lsp lua stdlib refactor changes that are not features or bugfixes labels Oct 17, 2021

gpanders mentioned this pull request Oct 17, 2021

fix: cleanup severity_sort handling in diagnostics #16046

Closed

gpanders changed the title ~~refactor(diagnostic)!: replace 'show_*' functions with 'preview'~~ refactor(diagnostic)!: replace 'show_*' functions with 'open_float' Oct 17, 2021

gpanders requested review from justinmk and mjlbach October 18, 2021 00:59

gpanders mentioned this pull request Oct 18, 2021

vim.diagnostic.goto_next / goto_prev doesn't automatically show diagnostics at the end of a line #16062

Closed

gpanders force-pushed the diagnostic-preview branch from 798daab to c62d0d6 Compare October 18, 2021 02:02

kylo252 reviewed Oct 18, 2021

View reviewed changes

runtime/doc/diagnostic.txt Show resolved Hide resolved

runtime/lua/vim/diagnostic.lua Outdated Show resolved Hide resolved

gpanders added this to the 0.6 milestone Oct 19, 2021

justinmk reviewed Oct 19, 2021

View reviewed changes

runtime/lua/vim/diagnostic.lua Outdated Show resolved Hide resolved

justinmk approved these changes Oct 19, 2021

View reviewed changes

gpanders force-pushed the diagnostic-preview branch 2 times, most recently from 1a97e2e to d94711b Compare October 19, 2021 14:53

mjlbach suggested changes Oct 19, 2021

View reviewed changes

Update docs

bbbba59

mfussenegger approved these changes Oct 19, 2021

View reviewed changes

Revert usage of deep_extend (see comment)

a12e335

gpanders commented Oct 19, 2021

View reviewed changes

gpanders merged commit 064411e into neovim:master Oct 19, 2021

gpanders mentioned this pull request Oct 19, 2021

Following HEAD: breaking changes on master #14090

Closed

gpanders deleted the diagnostic-preview branch October 19, 2021 17:46

Julian added a commit to Julian/dotfiles that referenced this pull request Oct 19, 2021

Tweak diagnostic mappings, mainly for neovim/neovim#16057

4651567

n3wborn added a commit to n3wborn/nvim that referenced this pull request Oct 19, 2021

Settings(diagnostics) : Update to use new open_float()

a837b74

See neovim/neovim#16057

n3wborn mentioned this pull request Oct 19, 2021

Settings(diagnostics) : Update to use new open_float() n3wborn/nvim#71

Merged

ray-x added a commit to ray-x/navigator.lua that referenced this pull request Oct 19, 2021

refactor diagnostic neovim/neovim#16057

fab0815

megalithic added a commit to megalithic/dotfiles that referenced this pull request Oct 25, 2021

fix(nvim,lsp): fixes nvim lsp diagnostic things

8a3703d

- nightly neovim/neovim#16057 changed how line diagnostics are shown/rendered.

fitrh added a commit to fitrh/init.nvim that referenced this pull request Oct 29, 2021

[ lsp/ui/diagnostic ] Add float key to config

d3c607c

ref: neovim/neovim#16057

fitrh added a commit to fitrh/init.nvim that referenced this pull request Oct 29, 2021

[ lsp/command/diagnostic ] Use float for diagnostic.goto_xxx

b687f1b

Replace `enable_popup` and `popup_opts`. ref: neovim/neovim#16057

AlwynYin mentioned this pull request Jan 29, 2022

show_line_diagnostics depreciated LunarVim/Neovim-from-scratch#74

Open

mrded added a commit to mrded/dotfiles that referenced this pull request Feb 18, 2022

fix: replace diagnostic.show_line_diagnostics with diagnostic.open_float

39d8323

see: neovim/neovim#16057

adrien-tagheuer added a commit to acharruel/dotfiles that referenced this pull request Apr 5, 2022

nvim: Fix deprecated call to show_line_diagnostic()

0021632

See explainations here: neovim/neovim#16057

Julian added a commit to Julian/dotfiles that referenced this pull request Jun 18, 2022

Tweak diagnostic mappings, mainly for neovim/neovim#16057

d6883a3

vincentqb mentioned this pull request Jan 18, 2023

Warning/Error line if too long, can't be read neovim/nvim-lspconfig#1046

Closed

ctlusto added a commit to ctlusto/dotfiles that referenced this pull request Sep 7, 2023

Update LSP diagnostics commands

1afff6e

There have been breaking changes to that API: ref neovim/neovim#16057

gpanders mentioned this pull request Sep 11, 2023

feat(treesitter): add a query editor #24703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(diagnostic)!: replace 'show_*' functions with 'open_float' #16057

refactor(diagnostic)!: replace 'show_*' functions with 'open_float' #16057

gpanders commented Oct 17, 2021 •

edited

clason commented Oct 17, 2021 •

edited

clason commented Oct 17, 2021 •

edited

gpanders commented Oct 17, 2021

mjlbach commented Oct 17, 2021 •

edited

mjlbach commented Oct 17, 2021

kylo252 commented Oct 17, 2021

gpanders commented Oct 17, 2021

mjlbach commented Oct 17, 2021 •

edited

justinmk commented Oct 17, 2021 •

edited

mjlbach commented Oct 17, 2021 •

edited

ckipp01 commented Oct 19, 2021

justinmk left a comment

mjlbach commented Oct 19, 2021

mjlbach left a comment •

edited

mfussenegger left a comment

mfussenegger Oct 19, 2021

gpanders Oct 19, 2021 •

edited

sQVe commented Oct 19, 2021 •

edited

gpanders commented Oct 19, 2021

sQVe commented Oct 19, 2021

gpanders commented Oct 19, 2021

n3wborn commented Oct 19, 2021

megalithic commented Oct 26, 2021

gpanders commented Oct 26, 2021

refactor(diagnostic)!: replace 'show_*' functions with 'open_float' #16057

refactor(diagnostic)!: replace 'show_*' functions with 'open_float' #16057

Conversation

gpanders commented Oct 17, 2021 • edited

clason commented Oct 17, 2021 • edited

clason commented Oct 17, 2021 • edited

gpanders commented Oct 17, 2021

mjlbach commented Oct 17, 2021 • edited

mjlbach commented Oct 17, 2021

kylo252 commented Oct 17, 2021

gpanders commented Oct 17, 2021

mjlbach commented Oct 17, 2021 • edited

justinmk commented Oct 17, 2021 • edited

mjlbach commented Oct 17, 2021 • edited

ckipp01 commented Oct 19, 2021

justinmk left a comment

Choose a reason for hiding this comment

mjlbach commented Oct 19, 2021

mjlbach left a comment • edited

Choose a reason for hiding this comment

mfussenegger left a comment

Choose a reason for hiding this comment

mfussenegger Oct 19, 2021

Choose a reason for hiding this comment

gpanders Oct 19, 2021 • edited

Choose a reason for hiding this comment

sQVe commented Oct 19, 2021 • edited

gpanders commented Oct 19, 2021

sQVe commented Oct 19, 2021

gpanders commented Oct 19, 2021

n3wborn commented Oct 19, 2021

megalithic commented Oct 26, 2021

gpanders commented Oct 26, 2021

gpanders commented Oct 17, 2021 •

edited

clason commented Oct 17, 2021 •

edited

clason commented Oct 17, 2021 •

edited

mjlbach commented Oct 17, 2021 •

edited

mjlbach commented Oct 17, 2021 •

edited

justinmk commented Oct 17, 2021 •

edited

mjlbach commented Oct 17, 2021 •

edited

mjlbach left a comment •

edited

gpanders Oct 19, 2021 •

edited

sQVe commented Oct 19, 2021 •

edited