-
-
Notifications
You must be signed in to change notification settings - Fork 636
feat(#1826): use diagnostics icons from vim.diagnostic #3190
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this one @igorlfs - this will be most useful.
The timing isn't working, however. At nvim-tree startup, diagnostics may not have been configured or even started.
Alternative Implementation
Retrieve the signs dynamically in DiagnosticsDecorator
. This will ensure that we pick up the configured signs, and allow the user to change them at runtime.
lua/nvim-tree.lua
Outdated
hint = get_diagnostics_icons(vim.diagnostic.severity.HINT) or "", | ||
info = get_diagnostics_icons(vim.diagnostic.severity.INFO) or "", | ||
warning = get_diagnostics_icons(vim.diagnostic.severity.WARN) or "", | ||
error = get_diagnostics_icons(vim.diagnostic.severity.ERROR) or "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the user copies and uses this default config this fails, as get_diagnostics_icons
is local to this file.
No matter, we will work around this another way.
Just pushed a commit following this implementation. LMK if that's not what you meant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good.
However, this will break existing users, as vim diagnostic config will override their diagnostics.icons
settings. We will need to add an option to allow the users to enable this if they choose.
*nvim-tree.diagnostics.diagnostic_opts*
|vim.diagnostic.Opts| overrides |nvim-tree.diagnostics.severity| and
|nvim-tree.diagnostics.icons|
Type: `boolean`, Default: `false`
Please add the severity functionality. Yes, that is scope creep, however we should make the user experience complete.
if type(signs) == "function" then | ||
signs = signs(0, 0) | ||
end | ||
return (type(signs) == "table" and signs.text and signs.text[severity]) or nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is safe and is returning correctly with the following test cases:
vim.diagnostic.config({
signs = function(namespace, bufnr)
return {
text = {
"e", "w", "i", "h",
}
}
end,
})
vim.diagnostic.config({
signs = {
text = {
"e", "w", "i", "h",
}
}
})
@@ -51,7 +62,7 @@ function DiagnosticsDecorator:new(args) | |||
self.diag_icons = {} | |||
for name, sev in pairs(ICON_KEYS) do | |||
self.diag_icons[sev] = { | |||
str = self.explorer.opts.diagnostics.icons[name], | |||
str = get_diagnostics_icons(sev) or self.explorer.opts.diagnostics.icons[name], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this more efficient and call get_diagnostics_icons
once in this constructor. It could return a single table with all severities populated.
We don't have to do exactly that; we just don't want to call vim.diagnostic.config()
and extract all the icons multiple times as it is expensive.
@@ -3,6 +3,17 @@ local diagnostics = require("nvim-tree.diagnostics") | |||
local Decorator = require("nvim-tree.renderer.decorator") | |||
local DirectoryNode = require("nvim-tree.node.directory") | |||
|
|||
---@param severity vim.diagnostic.Severity | |||
---@return string? | |||
local function get_diagnostics_icons(severity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this function a private member.
Closes #1826
Based on nvim-lualine/lualine.nvim#1091, which never got merged