Skip to content

feat(eslint): add vim.lsp.config support #3731

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

crnvl96
Copy link

@crnvl96 crnvl96 commented Apr 15, 2025

This PR crosses eslint from #3705

Compared locally against the current implementation from lua/lspconfig/configs/eslint.lua ans seems ok, but I'm open for more specific checks or any kind of improvement you may have.

@crnvl96 crnvl96 requested a review from glepnir as a code owner April 15, 2025 17:46
@@ -0,0 +1,202 @@
-- https://github.com/hrsh7th/vscode-langservers-extracted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing @brief. look at other configs

end, {})
end,
-- https://eslint.org/docs/user-guide/configuring/configuration-files#configuration-file-formats
root_dir = function(bufnr, on_dir)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest changing the root_dir field to root_markers.
Since there's a rule that the eslint config file must be in the root directory, I believe this change won't cause any problems.
Also, if you refer to lsp/ts_ls.lua, you can confirm that ts_ls also changed into root_markers field from root_dir.
we can just use root_file variable for root_markers. then it can be free from loading lspconfig-utils module also.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern with that change is that, although deprecated, it's possible to define an eslint config inside a package.json file.

This is handled dynamically by lspconfig, which checks for specific patterns in the user's package.json file. Since the property root_markers only accepts a list of strings, we would lose the ability to support package.json-based ESLint configs if migrating from root_dir.

The current root_dir implementation allows for this more complex pattern matching, while root_markers would be limited to exact file names.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think of that. Thank you.

@crnvl96 crnvl96 force-pushed the master branch 2 times, most recently from fd58769 to 17cd611 Compare April 16, 2025 13:07
@crnvl96 crnvl96 requested review from justinmk and dorage April 16, 2025 13:25
@marks0mmers
Copy link

Thanks for the work on this, I was just about to begin my attempt at this change, and then saw yours!

lsp/eslint.lua Outdated
local function fix_all(opts)
opts = opts or {}

local bufnr = util.validate_bufnr(0)
Copy link

@seblyng seblyng Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
local bufnr = util.validate_bufnr(0)
vim.validate('bufnr', bufnr, 'number')

Copy link
Author

@crnvl96 crnvl96 Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code uses util.validate_bufnr intentionally. This function acts as a compatibility layer, ensuring buffer number validation will be properly called depending on the neovim version being run.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vim.lsp.config was introduced in 0.11, meaning that this code should only run on 0.11, so it's a perfectly fine change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes, the obvious is so clear that it becomes hard to see 😅

My bad, and thank you

lsp/eslint.lua Outdated

if opts.sync then
request = function(buf, method, params)
client.request_sync(method, params, nil, buf)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
client.request_sync(method, params, nil, buf)
client:request_sync(method, params, nil, buf)

lsp/eslint.lua Outdated
end
else
request = function(buf, method, params)
client.request(method, params, nil, buf)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
client.request(method, params, nil, buf)
client:request(method, params, nil, buf)

@crnvl96 crnvl96 force-pushed the master branch 2 times, most recently from df5d577 to 05a7db0 Compare April 16, 2025 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants