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: add pylyzer dependenncy info (erg) (#3433) #3441

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

Conversation

fishBone000
Copy link

Problem: Pylyzer needs Erg as dependency, such info is missing in configs.md
Solution: Add such info in doc, and add ergPath setup option

Problem: Pylyzer needs Erg as dependency, such info is missing in
configs.md
Solution: Add such info in doc, and add `ergPath` setup option
Comment on lines 26 to 31
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,
Copy link
Member

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.

Copy link
Author

@fishBone000 fishBone000 Nov 19, 2024

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?

Copy link
Member

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.
@fishBone000
Copy link
Author

Will mark it ready for review after I test if it's working.

@fishBone000 fishBone000 marked this pull request as ready for review November 20, 2024 10:23
@fishBone000 fishBone000 requested a review from glepnir as a code owner November 20, 2024 10:23
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.

2 participants