-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: add pylyzer dependenncy info (erg) (#3433) #3441
base: master
Are you sure you want to change the base?
Conversation
Problem: Pylyzer needs Erg as dependency, such info is missing in configs.md Solution: Add such info in doc, and add `ergPath` setup option
lua/lspconfig/configs/pylyzer.lua
Outdated
ergPath = vim.fn.getenv('HOME') .. '/.erg', | ||
on_new_config = function(new_config, _) | ||
if new_config.ergPath then | ||
new_config.cmd_env.ERG_PATH = new_config.ergPath | ||
end | ||
end, |
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 saves the user basically nothing, while also adding a special-case interface to this config.
It would be more useful to just document what you're doing here, instead of adding a special ergPath
field.
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 thought the cmd_env
is gonna work? I found it in bqnlsp's default config.
I think it would still be nice if I set the ERG_PATH
in default config, so users who aren't familiar with lsp config can use it out of box (they still need to extract tarball to the path ofc):
on_new_config = function(new_config, _)
vim.fn.setenv("ERG_PATH", vim.fn.getenv("HOME") .. "/.erg")
end
Is this OK?
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.
cmd_env
is a standard field, no objection to using that. My objection is about the custom ergPath
thing.
still be nice if I set the
ERG_PATH
in default config, so users who aren't familiar with lsp config can use it out of box
Sure, yes. And some sort of brief hint in the docs.
Remove unnecessary `ergPath` field.
Will mark it ready for review after I test if it's working. |
Problem: Pylyzer needs Erg as dependency, such info is missing in configs.md
Solution: Add such info in doc, and add
ergPath
setup option