plugins/ltes-extra: support ltex_plus #3129
Conversation
|
I've added options to the Tests previously failed due to ltex.enable not being masked properly. That is now fixed |
First, thanks for the good work, this is looking great! |
|
@nix-community/nixvim can someone double-check this PR please? Espacially the |
ltex_plus is a maintained fork that recently got supported by ltex-extra barreiroleo/ltex_extra.nvim#66 Enable it unless the user explicitly use ltex
|
Changes based on feedback. Made the warning logic identify additional definitions of The formatting is a bit awkward for the warning. I guess it's sufficient this for temporary code, however. |
| inherit name; | ||
| inherit (e) file; | ||
| }; | ||
| external = foldlAttrs anyExternal null options.plugins.lsp.servers.ltex; |
There was a problem hiding this comment.
Interesting approach using the definition location 😎
I wonder if it'd be better to filter for external definitions, instead of reducing to the first match? That way we could list all offending definitions in the warning.
I'm also a little concerned about false-pisitives where there are irrelevant low-prio defs. I can't recall ottomh whether definitions includes all definitions or only definitions of the highestPrio? I can check this in nix repl when I'm next at a PC.
There was a problem hiding this comment.
Thanks, it was sort of inspired by your options priority-suggestion.
If I recall from my testing, it filters based on priority, but only at the top level. So if there are recursive attributes, such as in .settings, even irrelevant options appear.
In this particular case, it would basically only be encountered if there is a config with an onAttached.function = mkDefault ... as we're not really setting any other property. And I estimate that to be exceedingly rare.
There was a problem hiding this comment.
Another consideration is if/how mkIf false definitions show up. E.g. a user might have a toggle somewhere between a manual configuration of the lsp and using this plugin, implemented using mkIf.
| when = !lspCfg.servers.ltex.enable && external != null; | ||
| message = '' | ||
| in ${external.file} | ||
| You seem to have configured `plugins.lsp.servers.ltex.${external.name}` for `ltex-extra`. |
There was a problem hiding this comment.
If we have a reference to the option itself, we can interpolate it here.
"${options.foo}"
=> "programs.nixvim.foo"options come with a __toString (or was it outPath?) attr with the value lib.showOption option.loc, so interpolating them will stringify to the option path.
This has the advantage of automatically including path prefixes, such as programs.nixvim.
| It now uses `plugins.lsp.servers.ltex_plus` by default, | ||
| either move the configuration or explicitly enable `ltex` with `plugins.lsp.servers.ltex.enable = true` |
There was a problem hiding this comment.
I'm probably being a bit dense, but I struggle to follow what "it" is refering to. Also what/where the "configuration" should be moved.
Layout nit: I wonder if bullet points would be slightly clearer? If not, maybe we should consider an Oxford-comma before the "or"?
There was a problem hiding this comment.
Yeah, I struggled to layout that part for a bit. Good suggestions, I'll revisit this.
ltex_extra reacently got support for ltex_plus, a more updated and still maintained fork of ltex (barreiroleo/ltex_extra.nvim#66)
This PR adds support for both ltex, and ltex_plus, but defaults to using ltex_plus if neither is enabled. I've tried adding a warning of the change to notify users who depend on the implicit enabling of ltex.
I'm open to suggestions if there are other approaches to similar situations in Nixvim.