Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(diag): add diagnostics_update_on_event option #932

Merged
merged 2 commits into from
Jul 10, 2024

Conversation

gepbird
Copy link
Contributor

@gepbird gepbird commented Jun 25, 2024

About this PR

Closes #923.

This change makes the ui refresh whenever nvim reports a diagnostic change.

Demo: bufferline diagnostics are up to date

2024-06-25.14-24-39.mp4

Possible improvements for later

When nvim calls our diagnostic handler, we could get extra information about the change, for example the buffer that the change occurred in, all the details about the diagnostics in that buffer and options like update_in_insert (configured via vim.diagnostic.config). We could use these to replace vim.diagnostic.get call (possibly improving performance) and deprecate the bufferline option diagnostic_update_in_insert. However it would increase the complexity since we want to keep support for coc, so I decided not to do these changes.

@@ -657,6 +657,7 @@ local function get_defaults()
diagnostics = false,
diagnostics_indicator = nil,
diagnostics_update_in_insert = true,
diagnostics_update_on_event = false,
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can set this to true by default if they are using nvim_lsp and then update in insert to false. I'll look into deprecating that eventually

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, but shouldn't update in insert be set to what the user configured in nvim with vim.diagnostic.config? If so, should we display a warning when a user tries to set update in insert for bufferline config (when using nvim_lsp)?

Copy link
Owner

Choose a reason for hiding this comment

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

Tbh I'd like to deprecate the setting entirely since it now seems to be moot, if they use update on event then it will update in insert if they have set that in their lsp config. I don't get much time to do granular tweaks often to this plugin so more keen on trying to set the new normal whenever there are changes like this.

Maybe it's worth just add a deprecation warning that shows one time on plugin load to say this option is now essentially irrelevant because you should use update_on_event and make sure that your vim.diagnostic.config sets update on insert to true.

I think I have a mechanism in the code base already for deprecating options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of deprecated that option entirely, but what about coc users?

And yes a deprecation warning on startup would be the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I only deprecated it for nvim_lsp users, so there are no regressions.

@gepbird gepbird force-pushed the diagnostic-update-on-event branch 2 times, most recently from 77e6aff to d6b687a Compare June 26, 2024 16:42
@gepbird gepbird force-pushed the diagnostic-update-on-event branch from d6b687a to 8e4a87f Compare June 26, 2024 16:42
@gepbird gepbird requested a review from akinsho July 10, 2024 11:07
@akinsho
Copy link
Owner

akinsho commented Jul 10, 2024

Sorry about the delay @gepbird I had forgotten about this

@@ -181,6 +181,15 @@ local function setup_commands()
})
end

local function setup_diagnostic_handler(preferences)
if preferences.options.diagnostics == "nvim_lsp" and preferences.options.diagnostics_update_on_event then
Copy link
Owner

Choose a reason for hiding this comment

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

nit: this conditional is quite verbose could be local opts = preferences.options

@akinsho akinsho merged commit aa16daf into akinsho:main Jul 10, 2024
1 check passed
@gepbird gepbird deleted the diagnostic-update-on-event branch July 10, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: diagnostics can easily get out of date
2 participants