Skip to content
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

fix(svlangserver): don't use custom project setting resolution #1948

Merged
merged 2 commits into from
Jun 8, 2022

Conversation

williamboman
Copy link
Contributor

@henry-hsieh 👋 It seems like the server configuration for svlangserver included some custom exrc-as-json-style project settings loader, which I don't think should be part of the stock configuration provided by lspconfig, but instead provided by users themselves.

@henry-hsieh
Copy link
Contributor

Hi @williamboman,
The svlangserver relies LSP client (vscode, coc.nvim, etc.) to provide project local setting. I previously open an issue #1874 to request alternative project local settings because Neovim deprecate exrc after v0.7 (remind me if I'm wrong). Therefore, I created custom exrc-as-json-style project settings loader to do this trick.

I thought that the on_init function could be removed in lspconfig. However, I'll move the json loader on_init as suggestion in README of svlangserver unless lspconfig provides official project local settings loader similar to coc.nvim. Or most of SystemVerilog projects won't work in the default settings of svlangserver

Copy link
Contributor

@henry-hsieh henry-hsieh left a comment

Choose a reason for hiding this comment

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

As I mentioned in imc-trading/svlangserver#22, .svlangserver is used for root_dir detection.

Co-authored-by: HenryHsieh <[email protected]>
@williamboman
Copy link
Contributor Author

The svlangserver relies LSP client (vscode, coc.nvim, etc.) to provide project local setting.

I see. What kind of project-local settings does it need, are the default ones not sufficient? In the current on_init it seems like cmd, filetypes and settings are overridden. Since this function is invoked after the server has initialized, setting cmd and filetypes will not really achieve anything, so I assume it's the settings that is interesting here (btw I think there's a client.notify("workspace/didChangeConfiguration") missing at the end, to let the server know the settings have changed). If you want to achieve project-local settings via JSON, I'd personally recommend taking a look at https://github.com/tamago324/nlsp-settings.nvim, it'll most likely provide you with the mechanism you're looking for (if you don't want to use exrc).

@williamboman williamboman force-pushed the fix/svlangserver-cleanup branch from 8b91b05 to 5c3628a Compare June 8, 2022 14:52
@justinmk justinmk merged commit b99c853 into neovim:master Jun 8, 2022
@henry-hsieh
Copy link
Contributor

What kind of project-local settings does it need, are the default ones not sufficient?

If default ones mean exrc, it's not secure (that's why Neovim deprecated it).

In the current on_init it seems like cmd, filetypes and settings are overridden. Since this function is invoked after the server has initialized, setting cmd and filetypes will not really achieve anything,

I didn't notice this, thanks for pointing out.

so I assume it's the settings that is interesting here (btw I think there's a client.notify("workspace/didChangeConfiguration") missing at the end, to let the server know the settings have changed).

Yes, you're right. Every project may have there settings. In addition to finding project's source files, the projects usually need macros to correctly configure the source files (similar to pass -DMACRO to C/C++ projects).

If you want to achieve project-local settings via JSON, I'd personally recommend taking a look at https://github.com/tamago324/nlsp-settings.nvim, it'll most likely provide you with the mechanism you're looking for (if you don't want to use exrc).

This is what I need. Thanks!

@williamboman williamboman deleted the fix/svlangserver-cleanup branch June 8, 2022 16:12
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.

3 participants