Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

feat: use defined signs for diagnostics provider #331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mintelm
Copy link

@mintelm mintelm commented Nov 16, 2022

Hey, first of all, thanks for creating feline.nvim.

This pull requests tries to get the text field of the defined signs created by the LSP (i.e. DiagnosticSignError, DiagnosticSignWarn, DiagnosticSignInfo, DiagnosticSignHint) instead of using the hardcoded icons. The hardcoded icons are however used as a fallback.

@famiu
Copy link
Owner

famiu commented Nov 17, 2022

Not sure if it's a thing with my config or if it's an issue for everyone, but, it seems that if sign_define isn't used, LSP automatically defines 'E' for Error, 'W' for Warning and so on when the sign is needed. If that's the case for everyone, then that means the fallback signs would never be used.

@mintelm
Copy link
Author

mintelm commented Nov 17, 2022

This is true. I removed the fallback and also return '' instead of nil if no sign is defined, which should never be the case when using nvim-lspconfig (force-pushed).

However, now that I think about it, there might be some user that uses lsp without nvim-lspconfig and therefore no sign might be defined and the fallback would be used? I do not know how to test this, as I've never tried using lsp without nvim-lspconfig.

@famiu
Copy link
Owner

famiu commented Nov 17, 2022

This is true. I removed the fallback and also return '' instead of nil if no sign is defined, which should never be the case when using nvim-lspconfig (force-pushed).

However, now that I think about it, there might be some user that uses lsp without nvim-lspconfig and therefore no sign might be defined and the fallback would be used? I do not know how to test this, as I've never tried using lsp without nvim-lspconfig.

Seems like it's a Neovim core thing and thus is unaffected by whether nvim-lspconfig is used or not

@famiu
Copy link
Owner

famiu commented Nov 17, 2022

This is true. I removed the fallback and also return '' instead of nil if no sign is defined, which should never be the case when using nvim-lspconfig (force-pushed).

However, now that I think about it, there might be some user that uses lsp without nvim-lspconfig and therefore no sign might be defined and the fallback would be used? I do not know how to test this, as I've never tried using lsp without nvim-lspconfig.

Turns out we do need some form of fallback, since the signs are only defined only after diagnostics are shown, they are still undefined until then. Also using E, W, etc. as diagnostics icons would be really ugly and undesirable. Need to find a way around that as well

@mintelm
Copy link
Author

mintelm commented Nov 17, 2022

But as long as there are not diagnostics, the icon will never show, therefore it should be acceptable that the icon is undefined until then.

Also using E, W, etc. as diagnostics icons would be ugly indeed. However, Anyone who does not like that sign is supposed to redefine it anyway. I think using the defined sign actually improves uniformity, i.e. the icons in the sign column and statusline are the same.

Which is my usecase -- I want uniform icons across all UI elements. However, this can also be achieved using the icon field of the provider, therefore if you do not like relying on the signs because they are defined as E,W,etc. per default, I do not mind if the PR gets closed. I think it improves overall uniformity which is always good, but that's just my 2 cents.

@famiu
Copy link
Owner

famiu commented Nov 17, 2022

But as long as there are not diagnostics, the icon will never show, therefore it should be acceptable that the icon is undefined until then.

That is not true. There is nothing (except an enabled component item which is not mandatory) preventing diagnostics from being shown even if there are none.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants