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

Add support for running pyanalyze as a Pre-Commit hook #216

Open
CAM-Gerlach opened this issue Jul 26, 2021 · 11 comments
Open

Add support for running pyanalyze as a Pre-Commit hook #216

CAM-Gerlach opened this issue Jul 26, 2021 · 11 comments
Assignees

Comments

@CAM-Gerlach
Copy link
Contributor

As I'm sure you're aware, Pre-Commit is a popular Python/cross-language hooks framework for managing variety of checking, linting, formatting and other tools automatically, on demand in in CIs. Adding a minimal .pre-commit-hooks.yaml file in the root of the repo with a few lines of config will allow other projects to easily adopt pyanalyze, either locally or on CI, by simply pasting a couple-line snippet into their .pre-commit-config.yaml file, and pre-commit will install, run and update pyanalyze automagically, making it much easier to adopt, configure and maintain.

I'd be happy to add this in a PR if you don't object; I'd also document it with the standard snippit in the README. Once this goes in (as well as PR #215 ), it would be nice to have a release sometime soon so we can start taking advantage of it. Thanks!

@JelleZijlstra
Copy link
Contributor

Happy to take this! Similarly there should be a GitHub Action but I haven't gotten around to working on that.

@CAM-Gerlach
Copy link
Contributor Author

Thanks! Added the hook and working on testing and documenting it.

One question—what version do you plan to be the next release that will include this, so I can list the correct tag in the snippit documenting this in the README?

Also, once this goes live in a tagged version, it can be added to the hooks list on pre-commit.com so others can find it more easily.

As for a Github Action, that would be useful as well, though for users running it through pre-commit it will be run automatically with their existing pre-commit action, or through pre-commit.ci.

@JelleZijlstra
Copy link
Contributor

The next one should be 0.3.0. I imagine I'm going to just go with 0.x.0 for a while.

@CAM-Gerlach
Copy link
Contributor Author

After spending waaaay too much time playing around with this, since design pre-commit installs its hooks in an isolated env rather than mucking with the user's current one, and it seems that pyanalyze requires not only the project's own deps to be installed, but the local project itself (which isn't possible with additional_dependencies and would need to be re-installed every run), it makes sense to just make this a local hook instead that uses the user's current environment and pyanalyze install, which is simple enough to do and is what is recommended (though not required) with tools that use dynamic analysis like pylint.

So I guess this should be closed, unless you have any ideas? The big blocker is that I get ImportErrors with anything that imports my local package, since it isn't installed in pre-commit's env and AFAIK by design, there's no way to get pre-commit to do so in editable mode. Maybe I could get around that with relative imports, but I'd have to change all my code just for this and absolute imports are officially recommended over relative by PEP 8.

@JelleZijlstra
Copy link
Contributor

Thanks for looking into this! Yes, pyanalyze relies on actually importing the code it checks. Some sys.path hackery may make the pre-commit hook work, but I haven't tried myself. One of the items on my to-do list is to make it easier to run pyanalyze on arbitrary projects without too much sys.path hackery.

@atropos112
Copy link

Hello there @JelleZijlstra are there any plans to make the pre-commit for pyanalyze or has there been a decision made to not pursue this any further ?

@JelleZijlstra
Copy link
Contributor

I haven't looked at this recently. I can give it a try again at some point, but I'm not sure the design of pre-commit would interact well with pyanalyze.

@JelleZijlstra JelleZijlstra self-assigned this Jun 7, 2023
@lesleslie
Copy link

This seems to work for me (if pyanalyze is installed in PATH):

.pre-commit-config.yaml

- repo: local
  hooks:
    - id: pyanalyze
      name: pyanalyze
      entry: python -m pyanalyze
      language: python
      files: \.py$
      additional_dependencies:
        - pyanalyze>=0.10.1

pyproject.toml

[tool.pyanalyze]
paths = [
    "PROJECTPATH",
]

@JelleZijlstra
Copy link
Contributor

Thanks, I got that to work on https://github.com/JelleZijlstra/taxonomy, but found that I also have to list all the project's dependencies in additional_dependencies, so that pyanalyze can import the project. Wonder if there's a way to avoid having to duplicate the list of dependencies.

@lesleslie
Copy link

lesleslie commented Jun 18, 2023

I would maybe try installing your project as an editable dependency of itself. This is done automatically with PDM running
pdm update.

Adding it manually would be something like pdm add -e [full path to myproject] or pip install -e [full path to myproject].

Currently, I am using PDM with PEP-582 and pyanalyze seems to find the dependencies in my pypackages directory just fine when running pre-commit.

@CAM-Gerlach
Copy link
Contributor Author

This is pretty fundamental to the design of pre-commit; its primarily intended to be used with static analysis tools that don't require access to the runtime environment. However, it can be made to work with these use cases by either each user manually specifying their requisite additional_dependencies, or (probably better for most use cases) by making it a system hook and assuming Pyanalyze is installed in the local environment—this is what I did for my local Pyanalyze hook, and what Pylint does.

If users add pyanalyze as a dev dependency by whatever method they are already using for installing their other dev deps (including pre-commit itself), then making it a system hook "just works" (other than deferring the pyanalyze environment and version management to whatever other tooling your project/developers are using rather than pre-commit itself).

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

4 participants