LSP initialisation abstraction #73
Conversation
WalkthroughThe LSP client setup for multiple language servers, including Astro, Beancount, C/C++, and others, has been refactored from immediate, imperative configuration and registration calls to a declarative, table-based approach. Each client module now exports a configuration table with properties and optional activation logic, which are processed centrally in a loop for conditional setup and health registration. Changes
Sequence Diagram(s)sequenceDiagram
participant MainLSP as LSP Loader (lsp.lua)
participant Clients as LSP Client Config Modules
participant LSPConfig as lspconfig
participant Health as Health Register
MainLSP->>Clients: Import config tables for each client
loop For each client in clients
MainLSP->>client: Evaluate activate() (default true if absent)
alt activate() returns true
MainLSP->>LSPConfig: setup(client.name, combined config)
MainLSP->>Health: register_lsp(client.name)
end
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)`plugins/**/*.lua`: Please ensure all of the following: - Plugins are lazyloaded if possible through `nobbz.lazy` - `local` has to be used - coding follows the [luarocks styleguide...
🔇 Additional comments (11)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/nobbz/lua/nobbz/lsp.lua(2 hunks)plugins/nobbz/lua/nobbz/lsp/astro.lua(1 hunks)plugins/nobbz/lua/nobbz/lsp/beancount.lua(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`plugins/**/*.lua`: Please ensure all of the following: - Plugins are lazyloaded if possible through `nobbz.lazy` - `local` has to be used - coding follows the [luarocks styleguide...
plugins/**/*.lua: Please ensure all of the following:
- Plugins are lazyloaded if possible through
nobbz.lazylocalhas to be used- coding follows the luarocks styleguide, but with 2 spaces indentation
plugins/nobbz/lua/nobbz/lsp/astro.luaplugins/nobbz/lua/nobbz/lsp/beancount.luaplugins/nobbz/lua/nobbz/lsp.lua
🧠 Learnings (1)
plugins/nobbz/lua/nobbz/lsp.lua (1)
Learnt from: NobbZ
PR: NobbZ/nobbz-vim#19
File: plugins/nobbz/lua/nobbz/lsp/beancount.lua:3-6
Timestamp: 2025-01-31T23:06:17.133Z
Learning: In the nobbz-vim project, `LSP_CAPAS` is defined as a global variable in `plugins/nobbz/lua/nobbz/init.lua` and should be used for LSP capabilities across the codebase.
🔇 Additional comments (3)
plugins/nobbz/lua/nobbz/lsp/astro.lua (1)
3-6: LGTM! Clean declarative configuration.The refactor correctly transforms the imperative setup to a declarative configuration table that follows the new centralized pattern.
plugins/nobbz/lua/nobbz/lsp.lua (1)
48-51: LGTM! Clean client configuration approach.The declarative client table approach enables better modularity and conditional activation.
plugins/nobbz/lua/nobbz/lsp/beancount.lua (1)
16-24: LGTM! Robust activation logic with proper file existence checks.The configuration table correctly implements the declarative pattern with thorough activation checks that verify both directory structure and journal file existence.
| for _, client_config in ipairs(clients) do | ||
| local name = client_config.name | ||
| local activate = client_config.activate or function() return true end | ||
| local capabilities = client_config.capabilities or LSP_CAPAS | ||
| local on_attach = client_config.on_attach or { helpers.default, } | ||
| local init_options = client_config.init_options | ||
| local root_dir = client_config.root_dir | ||
|
|
||
| local setup = { | ||
| on_attach = helpers.combine(on_attach), | ||
| capabilities = capabilities, | ||
| init_options = init_options, | ||
| root_dir = root_dir, | ||
| } | ||
|
|
||
| if activate() then | ||
| lspconfig[name].setup(setup) | ||
|
|
||
| register_lsp(client_config.name) | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling around LSP setup calls.
The loop lacks error handling which could cause one failing client to disrupt the entire LSP initialization process.
for _, client_config in ipairs(clients) do
local name = client_config.name
local activate = client_config.activate or function() return true end
local capabilities = client_config.capabilities or LSP_CAPAS
local on_attach = client_config.on_attach or { helpers.default, }
local init_options = client_config.init_options
local root_dir = client_config.root_dir
local setup = {
on_attach = helpers.combine(on_attach),
capabilities = capabilities,
init_options = init_options,
root_dir = root_dir,
}
if activate() then
- lspconfig[name].setup(setup)
-
- register_lsp(client_config.name)
+ local ok, err = pcall(function()
+ lspconfig[name].setup(setup)
+ end)
+
+ if ok then
+ register_lsp(client_config.name)
+ else
+ vim.notify("Failed to setup LSP client: " .. name .. " - " .. tostring(err), vim.log.levels.ERROR)
+ end
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, client_config in ipairs(clients) do | |
| local name = client_config.name | |
| local activate = client_config.activate or function() return true end | |
| local capabilities = client_config.capabilities or LSP_CAPAS | |
| local on_attach = client_config.on_attach or { helpers.default, } | |
| local init_options = client_config.init_options | |
| local root_dir = client_config.root_dir | |
| local setup = { | |
| on_attach = helpers.combine(on_attach), | |
| capabilities = capabilities, | |
| init_options = init_options, | |
| root_dir = root_dir, | |
| } | |
| if activate() then | |
| lspconfig[name].setup(setup) | |
| register_lsp(client_config.name) | |
| end | |
| end | |
| for _, client_config in ipairs(clients) do | |
| local name = client_config.name | |
| local activate = client_config.activate or function() return true end | |
| local capabilities = client_config.capabilities or LSP_CAPAS | |
| local on_attach = client_config.on_attach or { helpers.default, } | |
| local init_options = client_config.init_options | |
| local root_dir = client_config.root_dir | |
| local setup = { | |
| on_attach = helpers.combine(on_attach), | |
| capabilities = capabilities, | |
| init_options = init_options, | |
| root_dir = root_dir, | |
| } | |
| if activate() then | |
| local ok, err = pcall(function() | |
| lspconfig[name].setup(setup) | |
| end) | |
| if ok then | |
| register_lsp(client_config.name) | |
| else | |
| vim.notify( | |
| "Failed to setup LSP client: " .. name .. " - " .. tostring(err), | |
| vim.log.levels.ERROR | |
| ) | |
| end | |
| end | |
| end |
🤖 Prompt for AI Agents
In plugins/nobbz/lua/nobbz/lsp.lua around lines 53 to 73, the loop setting up
LSP clients lacks error handling, risking that a failure in one client setup
disrupts the entire process. Wrap the lspconfig[name].setup(setup) call and the
subsequent register_lsp call inside a pcall or similar error-catching mechanism
to safely handle any errors. Log or handle errors appropriately to ensure the
loop continues initializing other clients even if one fails.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
plugins/nobbz/lua/nobbz/lsp.lua (1)
53-79: Add error handling around LSP setup calls.The loop lacks error handling which could cause one failing client to disrupt the entire LSP initialization process.
for _, client_config in ipairs(clients) do local name = client_config.name local activate = client_config.activate or function() return true end local capabilities = client_config.capabilities or LSP_CAPAS local on_attach = client_config.on_attach or { helpers.default, } local init_options = client_config.init_options local root_dir = client_config.root_dir local cmd = client_config.cmd local settings = client_config.settings local on_init = client_config.on_init local setup = { on_attach = helpers.combine(on_attach), capabilities = capabilities, init_options = init_options, root_dir = root_dir, cmd = cmd, settings = settings, on_init = on_init, } if activate() then - lspconfig[name].setup(setup) - - register_lsp(client_config.name) + local ok, err = pcall(function() + lspconfig[name].setup(setup) + end) + + if ok then + register_lsp(client_config.name) + else + vim.notify("Failed to setup LSP client: " .. name .. " - " .. tostring(err), vim.log.levels.ERROR) + end end end
🧹 Nitpick comments (2)
plugins/nobbz/lua/nobbz/lsp/mdx.lua (1)
3-7: Consider the impact of side effects during module loading.The
vim.filetype.addcall executes immediately when the module is loaded, regardless of whether the LSP is actually activated. This might be the intended behavior if the filetype mapping should always be available, but consider whether this aligns with the new declarative approach.plugins/nobbz/lua/nobbz/lsp.lua (1)
41-51: Consider lazy loading LSP client modules.According to the coding guidelines, plugins should be lazy loaded if possible through
nobbz.lazy. Since these LSP clients are loaded unconditionally, consider whether lazy loading would be beneficial here.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
plugins/nobbz/lua/nobbz/lsp.lua(2 hunks)plugins/nobbz/lua/nobbz/lsp/c-cpp.lua(1 hunks)plugins/nobbz/lua/nobbz/lsp/digestif.lua(1 hunks)plugins/nobbz/lua/nobbz/lsp/elixir.lua(1 hunks)plugins/nobbz/lua/nobbz/lsp/gleam.lua(1 hunks)plugins/nobbz/lua/nobbz/lsp/html.lua(1 hunks)plugins/nobbz/lua/nobbz/lsp/lua.lua(1 hunks)plugins/nobbz/lua/nobbz/lsp/mdx.lua(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`plugins/**/*.lua`: Please ensure all of the following: - Plugins are lazyloaded if possible through `nobbz.lazy` - `local` has to be used - coding follows the [luarocks styleguide...
plugins/**/*.lua: Please ensure all of the following:
- Plugins are lazyloaded if possible through
nobbz.lazylocalhas to be used- coding follows the luarocks styleguide, but with 2 spaces indentation
plugins/nobbz/lua/nobbz/lsp/gleam.luaplugins/nobbz/lua/nobbz/lsp/digestif.luaplugins/nobbz/lua/nobbz/lsp/mdx.luaplugins/nobbz/lua/nobbz/lsp/html.luaplugins/nobbz/lua/nobbz/lsp/c-cpp.luaplugins/nobbz/lua/nobbz/lsp/elixir.luaplugins/nobbz/lua/nobbz/lsp.luaplugins/nobbz/lua/nobbz/lsp/lua.lua
🧠 Learnings (1)
plugins/nobbz/lua/nobbz/lsp.lua (1)
Learnt from: NobbZ
PR: NobbZ/nobbz-vim#19
File: plugins/nobbz/lua/nobbz/lsp/beancount.lua:3-6
Timestamp: 2025-01-31T23:06:17.133Z
Learning: In the nobbz-vim project, `LSP_CAPAS` is defined as a global variable in `plugins/nobbz/lua/nobbz/init.lua` and should be used for LSP capabilities across the codebase.
🔇 Additional comments (3)
plugins/nobbz/lua/nobbz/lsp/elixir.lua (1)
3-7: LGTM! Clean refactor to declarative configuration.The refactor from imperative LSP setup to declarative configuration table is well-executed. The code follows the luarocks style guide with proper 2-space indentation and uses
localappropriately.plugins/nobbz/lua/nobbz/lsp/lua.lua (2)
3-13: LGTM! Proper autoformatting setup.The
on_attachfunction correctly sets up synchronous formatting on save, which is appropriate for Lua files to ensure consistent formatting.
41-50: Excellent declarative configuration structure.The configuration table properly combines the default helper with the custom
on_attachfunction and includes appropriate Lua-specific settings. The structure aligns perfectly with the new LSP initialization pattern.
| return { | ||
| name = "mdx_analyzer", | ||
| on_attach = { helpers.keymap, }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing lazyloading mechanism as required by coding guidelines.
The declarative configuration approach is consistent with the refactoring pattern. However, the coding guidelines specify that plugins should be lazyloaded through nobbz.lazy, which is not implemented here.
🤖 Prompt for AI Agents
In plugins/nobbz/lua/nobbz/lsp/mdx.lua around lines 9 to 12, the plugin
configuration lacks the required lazyloading mechanism as per coding guidelines.
Modify the return table to include lazyloading by wrapping the plugin setup with
`nobbz.lazy` or by adding a lazyload key or function according to the project's
lazyloading pattern. This ensures the plugin is loaded only when needed,
following the established conventions.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors individual language server configurations into a unified, declarative abstraction that returns configuration tables. It centralizes LSP setup and defers health registration and activation to a global aggregator.
- Converted individual lspconfig setups to return configuration tables.
- Removed per-server health registration in favor of centralized registration.
- Introduced an aggregator in lsp.lua that loops through each server configuration for unified setup.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| plugins/nobbz/lua/nobbz/lsp/zig.lua | Refactored Zig LSP setup to return a config table instead of directly calling setup. |
| plugins/nobbz/lua/nobbz/lsp/typescript.lua | Similar refactor applied to the TypeScript LSP configuration. |
| plugins/nobbz/lua/nobbz/lsp/tailwind.lua | Converted Tailwind CSS LSP initialization to return configuration for centralized usage. |
| plugins/nobbz/lua/nobbz/lsp/rust.lua | Updated Rust LSP config with inline plugin specs and combined on_attach callbacks. |
| plugins/nobbz/lua/nobbz/lsp/python.lua | Updated Python LSP setup to conform to the new abstraction. |
| plugins/nobbz/lua/nobbz/lsp/oxide.lua | Reworked Oxide LSP configuration to return a config table with capabilities explicitly set. |
| plugins/nobbz/lua/nobbz/lsp/nushell.lua | Adjusted Nushell LSP setup to match centralized configuration standards. |
| plugins/nobbz/lua/nobbz/lsp/nil.lua | Modified Nil LSP configuration to return a config table. |
| plugins/nobbz/lua/nobbz/lsp/meson.lua | Refactored Meson LSP config to return a configuration table. |
| plugins/nobbz/lua/nobbz/lsp/mdx.lua | Converted MDX LSP setup to return a configuration table. |
| plugins/nobbz/lua/nobbz/lsp/lua.lua | Overhauled Lua LSP config to use local helper functions and return a config table. |
| plugins/nobbz/lua/nobbz/lsp/html.lua | Refactored HTML LSP config to return a configuration table. |
| plugins/nobbz/lua/nobbz/lsp/gleam.lua | Updated Gleam LSP configuration to return a configuration table. |
| plugins/nobbz/lua/nobbz/lsp/elixir.lua | Converted Elixir LSP setup to adhere to the new abstraction. |
| plugins/nobbz/lua/nobbz/lsp/digestif.lua | Refactored Digestif LSP setup to return a configuration table. |
| plugins/nobbz/lua/nobbz/lsp/c-cpp.lua | Adjusted C/C++ LSP configuration to return a configuration table for centralized setup. |
| plugins/nobbz/lua/nobbz/lsp/beancount.lua | Introduced an activate function in Beancount LSP config to conditionally enable it. |
| plugins/nobbz/lua/nobbz/lsp/astro.lua | Refactored Astro LSP configuration to return a configuration table. |
| plugins/nobbz/lua/nobbz/lsp.lua | Implemented a centralized aggregator that iterates over LSP configuration tables. |
Comments suppressed due to low confidence (1)
plugins/nobbz/lua/nobbz/lsp/lua.lua:3
- [nitpick] Consider renaming the local function 'on_attach' (e.g., 'custom_on_attach') to reduce ambiguity with the on_attach array in the returned table.
local function on_attach(_, buffer)
Summary by CodeRabbit