-
Notifications
You must be signed in to change notification settings - Fork 788
feat: add NullLsToggle command #1188
base: main
Are you sure you want to change the base?
Conversation
lua/null-ls/init.lua
Outdated
nargs = 1, | ||
complete = function() | ||
local list = {} | ||
for _, source in ipairs(M.get_sources()) do |
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.
Should this complete with sources active for the current filetype?
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.
I made it for all registered sources for the current filetype, since it needs to re-enable them again.
complete = function() | ||
local list = {} | ||
for _, source in ipairs(M.get_sources()) do | ||
list[#list + 1] = source.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.
I think there's a possibility of duplicate sources here, e.g. if the user has eslint
registered for both formatting and diagnostics.
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 will depend on how source.toggle()
should behave, and since it's only eslint
, I think it's probably fine to leave it like this?
otherwise, we'd need something similar to how LspStop
handles nargs
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.
If we just pass eslint
to source.toggle()
it'll toggle all sources matching the given query, which in this case means it'll toggle all sources named eslint
. I think this would be the least surprising behavior for users, and there's at least a few other sources that share names, so I think we should deduplicate them.
lua/null-ls/info.lua
Outdated
"Inactive source(s)", | ||
} | ||
|
||
for _, source in ipairs(sources.get(ft)) do |
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.
I haven't taken a close look, but is this logic okay? When I have multiple active sources and toggle them, I only see the first source under Inactive source(s)
.
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.
@jose-elias-alvarez, sorry for the delay! turned out to be two separate issues.. 😄
It should be fixed now!
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.
Sorry for the review back-and-forth on this! I found one bug but other than that I think this looks good.
end | ||
inactive_sources_info = create_inactive_sources_info(filetype) |
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.
I think both of these should also be under the is_attached
condition, right? At the moment the create_logging_info()
call is causing an error for me when the client isn't attached.
add
NullLsToggle
command with completion support