-
-
Notifications
You must be signed in to change notification settings - Fork 303
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
plugins/none-ls: switch to mkNeovimPlugin #1727
plugins/none-ls: switch to mkNeovimPlugin #1727
Conversation
ed93b84
to
630cc82
Compare
630cc82
to
773c2d8
Compare
plugins/none-ls/settings.nix
Outdated
# TODO: Upstream's description: | ||
# description = '' | ||
# Defines a list of sources for null-ls to register. | ||
# Users can add built-in sources (see [BUILTINS]) or custom sources (see [MAIN]). | ||
# | ||
# If you've installed an integration that provides its own sources and aren't | ||
# interested in built-in sources, you don't have to define any sources here. The | ||
# integration will register them independently. | ||
# | ||
# [BUILTINS]: https://github.com/nvimtools/none-ls.nvim/blob/main/doc/BUILTINS.md | ||
# [MAIN]: https://github.com/nvimtools/none-ls.nvim/blob/main/doc/MAIN.md | ||
# ''; |
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.
Same, this should be dealt with here I think.
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 for future reference, currently that option should only be used by the sources.**.enable
option implementation, hence visible=false
and keeping the previous description.
In the future this option may be useful for custom and/or third-party sources, so we'll likely take upstream's description into account.
I could remove the commented out upstream description if you like?
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.
Since the option is visible = false
anyway, I've included both in the description and removed the commented out one.
# This is implied: | ||
# enableLspFormat = true; |
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.
Just remove it entirely then, no ?
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.
Could do, but since this is kind of a weird option (default = plugins.lsp-format.enable
) and also an option we might remove/rework in the future, I thought it was useful to note the implication.
Up to you.
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.
No it's fine like this.
773c2d8
to
ef905b5
Compare
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.
LGTM
# This is implied: | ||
# enableLspFormat = true; |
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.
No it's fine like this.
ef905b5
to
ca8ac5f
Compare
Part one refactoring none-ls (#1705): switch the top-level plugin options to use mkNeovimPlugin and rfc42-style settings.
lsp-format
This PR maintains the existing option
enableLspFormat
, a legacy option that sets none-ls'son_attach
torequire('lsp-format').on_attach
. There are some issues with this option, but removing or refactoring it is not a focus for this initial PR.I did update the option description though: fixes #1690.
with()
argsThe various sources have a
withArgs
option (plugins.none-ls.sources.*.*.withArgs
), which is a lua-string type. Switching this to rfc42-style settings would be nice, but is not in scope for this PR. (Upstream docs, experimental draft PR).Non-builtin sources
The
settings.sources
option (previouslysourcesItems
) can list builtin sources, along with custom sources and "third-party" sources. We're currently only considering builtin sources (as listed inbuiltins.json
). Properly supporting other sources is not in scope for this PR.