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

cached lookup of mypy executable/venv based on document.path and workspace #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

petergaultney
Copy link

The general idea here is that Python programmers can probably think of a hundred different ways to dynamically configure which install of mypy gets used and how it gets run. I'm doing this by having pylsp-mypy look for a script called get-venv.py (which probably needs a different, more general/descriptive name, and/or could potentially have a configurable name/path via standard python-language-server config), and if it exists, it calls it with a JSON payload of the document path and workspace path, and processing the output as a simple JSON payload to modify how mypy is called.

An example get-venv.py script is shown below (a simplified version of my own). In my case, I always have mypy installed within the venvs themselves, so the simplest option for me is to use my venv manager to run mypy.

Other people might have different ways of setting up their config dynamically - in most cases I think they would be able to just add more items to the returned list, though in some cases they might end up wishing to modify the full invocation, which currently is not possible because I haven't provided it to the get-venv.py script. That would be easy enough to change.

#!/usr/bin/env python
import argparse
import json
import os
import typing as ty

POETRY = ("poetry", "run", "mypy")
PIPENV = ("pipenv", "run", "mypy")


def get_correct_invocation(cmd: ty.List[str], path: str, workspace: str = ""):
    assert isinstance(cmd, list), f"Command must be a list of strings but is {cmd}"
    if cmd == ["mypy"]:
        if path.startswith(os.path.expanduser("~/work/COPIES/ds-monorepo/")):
            return dict(cmd=POETRY, path=path, workspace=workspace)
        if path.startswith(os.path.expanduser("~/work/")):
            return dict(cmd=PIPENV, path=path, workspace=workspace)
    return dict(cmd=cmd, path=path, workspace=workspace)


def main():
    parser = argparse.ArgumentParser()
    parser.add_argument("json")
    args = json.loads(parser.parse_args().json)

    print(json.dumps(get_correct_invocation(args["cmd"], args["path"], args["workspace"])))


if __name__ == "__main__":
    main()

@@ -164,6 +166,23 @@ def pylsp_lint(
return get_diagnostics(config, workspace, document, settings, is_saved)


@lru_cache(maxsize=2000)
Copy link
Author

Choose a reason for hiding this comment

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

this size is pretty arbitrary but also pretty safe in terms of memory usage. we definitely want to cache these results so we're not constantly calling out to another process.

@@ -164,6 +166,23 @@ def pylsp_lint(
return get_diagnostics(config, workspace, document, settings, is_saved)


@lru_cache(maxsize=2000)
def get_venv_cmd(path: str, workspace: str) -> List[str]:
if not shutil.which("get-venv.py"):
Copy link
Author

Choose a reason for hiding this comment

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

again, this executable name can/probably should be configurable.

return []
try:
p = subprocess.run(
["get-venv.py", json.dumps(dict(cmd=["mypy"], path=path, workspace=workspace))],
Copy link
Author

Choose a reason for hiding this comment

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

in theory, we could send the entire args list as well - this would provide maximum flexibility at a cost of forcing the end user to process and return the received list a bit more carefully.

@Richardk2n
Copy link
Member

Is there any reason, why this is a separate file executed using subprocess.run and not just a normal part of the code?

@petergaultney
Copy link
Author

yes, the idea is that it allows users to write any kind of logic they want for determining the python environment based on the path of the file being checked.

For poetry and pipenv, something like my code could easily be standard, but this approach lets users essentially write arbitrary config for their own setups.

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