Skip to content

Conversation

@15cm
Copy link
Contributor

@15cm 15cm commented Feb 8, 2020

Different environment is supported for pyls in
palantir/python-language-server#680. Try to get the
environment specified by pyenv and pass it in.

15cm added 2 commits February 7, 2020 23:07
Different environment is supported for pyls in
palantir/python-language-server#680. Try to get the
environment specified by pyenv and pass it in.
@15cm 15cm requested a review from yyoncho February 8, 2020 07:54
@yyoncho yyoncho requested a review from mpanarin February 8, 2020 08:00
Copy link
Member

@yyoncho yyoncho left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

Here it is my review which mainly contains stylistic comments. I have no expertise in python and I will let @mpanarin comment/decide if your approach is ok. I think that it will be useful if we have a wiki/tutorial describing how this works. I am fine if you want to link your blog, etc.

- For Custom settings PROPS, adds doc for lambda function support and strips a
prop if it is not boolean and is [evaluated to be] nil.
- Add an option `lsp-pyls-plugins-jedi-environment` to set pyls environment.
- Move `lsp-pyls-plugins-jedi-use-pyenv-environment` check into the lambda
function so it will be evaluated per workspace.
- Style fixes.
Copy link
Member

@yyoncho yyoncho left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the comments!

One more nit after the restructuring: you may now merge both lsp-register-custom settings since after moving (when lsp-pyls-plugins-jedi-use-pyenv-environment inside the lambda it makes more sense to merge them.

I am approving the PR and we will be waiting for @mpanarin's review.

Copy link
Collaborator

@mpanarin mpanarin left a comment

Choose a reason for hiding this comment

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

Thanks for the great PR!

A few things though

Copy link
Collaborator

@mpanarin mpanarin left a comment

Choose a reason for hiding this comment

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

Okay, I'm fine with this. Although, imo, lsp-mode should be decoupled from python versioning (at least later on).
Please, make a wiki post on how to use this feature

We should look for good pyenv module for emacs that will handle that, instead of trying to support pyenv inside lsp-mode. Because if we support pyenv, then we should also think about asdf or other tools, which is obviously a road to nowhere.

@yyoncho please have a quick look on changes to lsp-register-custom-settings. Otherwise it is ready to be merged

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