plugins/lsp: migrate to mkNeovimPlugin #2700
Conversation
HeitorAugustoLN
force-pushed
the
lsp-migrate
branch
2 times, most recently
from
9225cec to
f07663d
Compare
|
|
||
| inherit (lib) mkOption types; |
There was a problem hiding this comment.
| inherit (lib) mkOption types; | |
| inherit (lib.nixvim) defaultNullOpts mkNullOrOption; | |
| inherit (lib) mkOption types; |
Just at a glance, this will allow you to get rid of lib.nixvim. in this file.
There was a problem hiding this comment.
It looks like there is more than that
There was a problem hiding this comment.
I'm in favour of inheriting anything that is used a lot. I'm not against being explicit, but defaultNullOpts for example gets used a ton.
| preConfig = mkOption { | ||
| type = types.lines; | ||
| description = "Code to be run before loading the LSP. Useful for requiring plugins"; | ||
| default = ""; |
There was a problem hiding this comment.
I wonder if preConfig and postConfig can be rename-alias modules pointing to lubConfig.pre and luaConfig.post respectively?
There was a problem hiding this comment.
I think it could, but it would need a little refactor
| ]; | ||
| default = [ ]; | ||
| lspBuf = mkOption { | ||
| type = with types; attrsOf (either str (attrsOf anything)); |
There was a problem hiding this comment.
I'm thinking we can a) use type coercion to reduce the post-processing and b) use our standard lib.nixvim.keymaps.mapOptionSubmodule for increased consistency.
We may need to extend mapOptionSubmodule to support key defaulting to the attr-name though.
I'm thinking something like
type = coercedTo str (action: {inherit action;}) mapOptionSubmodule;(Although we'd actually need to use the map-option submodule factory to produce a slightly customised type)
There was a problem hiding this comment.
I like the idea, but I don't know if it would be possible to make it backwards-compatible. But if it is not a problem, then I think this is better.
There was a problem hiding this comment.
When migrating plugins to mkNeovimPlugin it's acceptable to introduce some breaking changes. You can mark the old option as removed with instructions on how to migrate
There was a problem hiding this comment.
You can mark the old option as removed with instructions on how to migrate
This only really applies when the new option has a different name/location.
If the old "removed" option has the same name as the new option that doesn't work.
In this case, I think we can do a migration without breaking changes. But since the option isn't being renamed, it can wait until after this PR.
There was a problem hiding this comment.
Yeah I meant for options that we move from the top-level to settings, but if this option does not move then there's not much we can do
HeitorAugustoLN
force-pushed
the
lsp-migrate
branch
2 times, most recently
from
fd103de to
73d5833
Compare
HeitorAugustoLN
force-pushed
the
lsp-migrate
branch
from
73d5833 to
923bdc1
Compare
It should probably be fine, since I didn't change anything besides support for mode