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

lsp-pyright-venv-path vs. lsp-pyright-venv-directory? #49

Open
joostkremers opened this issue Apr 13, 2021 · 3 comments
Open

lsp-pyright-venv-path vs. lsp-pyright-venv-directory? #49

joostkremers opened this issue Apr 13, 2021 · 3 comments

Comments

@joostkremers
Copy link

Yesterday's commit 3fd23f1 added the option lsp-pyright-venv-directory, but there was already an option lsp-pyright-venv-path, which, judging by the description, has the same function.

BTW, both options have an incomplete :type spec. Since they can also be nil, the type should be something like this:

(defcustom lsp-pyright-venv-path nil
  "Path to folder with subdirectories that contain virtual environments.
Virtual Envs specified in pyrightconfig.json will be looked up in this path."
  :type '(choice (string :tag "Venv path")
                 (const :tag "No venv path" nil))
  :group 'lsp-pyright)

That way, they are displayed correctly in the Customize buffer and can be properly type-checked.

@seagle0128
Copy link
Collaborator

lsp-pyright-venv-path is absolute path, while lsp-pyright-venv-directory is the folder name.

See:

(defun lsp-pyright-locate-venv ()
  "Look for virtual environments local to the workspace."
  (or lsp-pyright-venv-path
      (and lsp-pyright-venv-directory
           (-when-let (venv-base-directory (locate-dominating-file default-directory lsp-pyright-venv-directory))
             (concat venv-base-directory lsp-pyright-venv-directory)))
      (-when-let (venv-base-directory (locate-dominating-file default-directory "venv/"))
        (concat venv-base-directory "venv"))
      (-when-let (venv-base-directory (locate-dominating-file default-directory ".venv/"))
        (concat venv-base-directory ".venv"))))

@joostkremers
Copy link
Author

Ah, thanks for the clarification. Admittedly, I didn't look at the code, just at the doc strings, which didn't make the difference clear to me. If you would like, I could prepare a PR to suggest an update to the doc strings.

@seagle0128
Copy link
Collaborator

PR is welcome

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

No branches or pull requests

2 participants