Pre-commit config is impossible

[Goldziher]

Hi there,

I am the author of [Starlite][https://github.com/starlite-api/starlite] and a user requested we support pyright better. I'm trying to set it up as part of our pre-commit configuration, which is crucial for our development, but this is really impossible.

I have this in my .pre-commit-config.yaml:

  - repo: local
    hooks:
      - id: pyright
        name: pyright
        entry: pyright
        language: node
        pass_filenames: false
        types: [python]
        additional_dependencies: ["pyright@1.1.269"]

The issue is that I cannot specify any additional dependency in this way because the language is node and not python. This causes pyright to not find any of the dependencies in place.

I would like to ask for an official pre-commit-config that fixes this issue.

[Goldziher]

So, I got it to work doing this:

  - repo: local
    hooks:
      - id: pyright
        name: pyright
        entry: poetry run pyright -p .
        language: system
        pass_filenames: false
        types: [python]

• note the above is using poetry to execute pyright, and it requires adding a config file.

BUT - for the above to work the user needs to have node installed. This means it will either not work in our CI (we use pre-commit in CI), or we will need to inject node there. Additionally, all contributors must have a compatible node version in place for this to work, and we have to add pyright into our dev dependencies.

I would suggest adding a pre-commit-config that uses a custom script, which makes sure everything is in place correctly.

Additionally - we cannot use the autoupdate functionality of pre-commit with the above, and pre-commit-ci will not be able to update it. So A solution in this repo is the best solution.

[Goldziher]

Ok, third time is the charm. This is a correct pre-commit config that actually used the pre-commit capabilities:

  - repo: local
    hooks:
      - id: pyright
        name: pyright
        entry: pyright
        language: python
        types: [python]
        additional_dependencies:
          [
            pyright,
            # required dependencies for type checking
            brotli,
            cryptography,
            freezegun,
            hypothesis,
            mako,
            orjson,
            piccolo,
            picologging,
            pydantic,
            pydantic_factories,
            pydantic_openapi_schema,
            pytest,
            requests,
            sqlalchemy,
            starlette,
            types-PyYAML,
            types-freezegun,
            types-redis,
            types-requests,
          ]

As you can see in the above I am able to add the additional_requirements, and these will be installed in the venv created by pre-commit. Additionally, they will not need to be included in my .lock file.

Given that the above works, it shouldnt be a problem to add an official pre-commit hook configuration in the root of this repository. If you give me the go ahead, I can do this quickly.

[erictraut]

Please refer to the discussion in this thread.

The approach you're proposing is problematic because it's referencing the python wrapper for pyright. This wrapper is not part of this repo and is not maintained by the pyright team itself, so I don't think it would make sense for this repo to provide a pre-commit hook that references it.

[Goldziher]

Please refer to the discussion in this thread.

The approach you're proposing is problematic because it's referencing the python wrapper for pyright. This wrapper is not part of this repo and is not maintained by the pyright team itself, so I don't think it would make sense for this repo to provide a pre-commit hook that references it.

Thank you, I saw this thread before posting this issue. I also refered to your docs in trying to set this up.

I'm afraid the fact of the matter is that the setup proposed in the docs of pyright simply does not work. That is, unless the codebase in question has no dependencies that are required for type checking.

I spent some time fixing this, and this is whats posted above. At a minimum, I would suggest you add a section in the docs pointing to the python wrapper and the pre-commit hook it offers rather than what is there now, because I spent some time on this.

I would actually hope to see a pythonic wrapper as part of this project - this being, after all, a tool in the python tool-chain.

Furhtermore, the documentation assumes that most users will use github actions. This is not a very good approach in my opinion for several reasons - 1. There are other CI options other than github. 2. pre-commit is a very common and useful tool in the python ecosystem. This being a python tool, even though its written in TS, I would have hoped for native support for it (see my hope above).

Thanks for your time.

[erictraut]

I agree with your point that the pyright CI documentation is misleading, so I'm on board with updating the documentation to prevent confusion. At a minimum, I'm thinking that we should delete the current section titled Running Pyright as a pre-commit hook.

It looks like the pyright-python wrapper already has a pre-commit hook. Have you tried following the steps documented on that page? Does that suit your needs? Perhaps it would be best for pyright's docs to simply link to the pyright-python docs for pre-commit hook configuration.

You mentioned that you needed to add additional dependencies to make this work. These dependencies are highly specific to your project, so it wouldn't make sense to include them in general documentation. Each project would need a different set of python libraries for its own type checking purposes. This will typically be the same libraries that are needed to run the project (those in the requirements.txt file, for example), but additional stub libraries may also be required. I noticed that you included a number of libraries that are already represented in typeshed (e.g. sqlalchemy, PyYAML, freezegun, redis, requests). Pyright bundles all typeshed stubs, so these shouldn't be required as additional dependencies. I also noticed that you didn't include version numbers for any of the dependencies. Was that intentional? Wouldn't you want to pin the versions to prevent library updates from unexpectedly breaking your pre-commit script?

I ask the above questions because if we're going to provide documentation for pre-commit configuration, I want to make sure that we're providing guidance that is complete and sets people up for success.

@jakebailey, let me know if you have any thoughts here.

[jakebailey]

Trying the pyright-python pre-commit is what I would do; I suggested it in the other thread.

I don't really know why you'd need to re-specify all of the dependencies again in the hook, though. I would expect that those are already installed and accessible, unless there's something wrong with the way pre-commit runs hooks (but I would expect not; clearly people can do things like testing with pre-commit).

[Goldziher]

I agree with your point that the pyright CI documentation is misleading, so I'm on board with updating the documentation to prevent confusion. At a minimum, I'm thinking that we should delete the current section titled Running Pyright as a pre-commit hook.

Yup

It looks like the pyright-python wrapper already has a pre-commit hook. Have you tried following the steps documented on that page? Does that suit your needs? Perhaps it would be best for pyright's docs to simply link to the pyright-python docs for pre-commit hook configuration.

I have and its working, with some minor issues. I would defintely suggest as a starting point to point this this - because its quite easy. Also, an official pre-commit is simply a yaml file in the root of the repository, which can also point at that wrapper.

You mentioned that you needed to add additional dependencies to make this work. These dependencies are highly specific to your project, so it wouldn't make sense to include them in general documentation. Each project would need a different set of python libraries for its own type checking purposes. This will typically be the same libraries that are needed to run the project (those in the requirements.txt file, for example), but additional stub libraries may also be required. I noticed that you included a number of libraries that are already represented in typeshed (e.g. sqlalchemy, PyYAML, freezegun, redis, requests). Pyright bundles all typeshed stubs, so these shouldn't be required as additional dependencies. I also noticed that you didn't include version numbers for any of the dependencies. Was that intentional? Wouldn't you want to pin the versions to prevent library updates from unexpectedly breaking your pre-commit script?

i can remove some of them, but some are required - otherwise i get type issues.

I think I need to clarify how pre-commit works- this tool creates an isolated venv for its python dependencies, separate from the project venv. It then executes against it. This is extremely useful - especially if you use lock files, because it keeps the stuff out of your lock file. It does mean though you need to install these dependencies in your pre-commit

As for your question regarding pinning versions - I dont do this. As a library maintainer I need to catch issues as they appear, which means I need to test against every new release. Pre-commit has an autoupdate option, but it doesn't update additional_dependencies. I need to run against the very latest version to catch issues when they appear.

I ask the above questions because if we're going to provide documentation for pre-commit configuration, I want to make sure that we're providing guidance that is complete and sets people up for success.

You absolutely dont need to specify my additional dependencies, I added these above to show you what I have in place.

If you are curious, you can see our config here: https://github.com/starlite-api/starlite/blob/main/.pre-commit-config.yaml

I will also point you at this: https://pre-commit.ci/, although I personally dont use it (because i dont like giving github access to 3rd party tools), its extremely useful and some of the largest open source python projects use it.

Cheers.

[jakebailey]

Also, an official pre-commit is simply a yaml file in the root of the repository, which can also point at that wrapper.

It isn't actually that simple. See: https://github.com/microsoft/pyright/issues/3612#issuecomment-1230590045

[jakebailey]

Oh, you mean to literally point to the third party repo from our repo. I think I'd rather people just use the third party repo directly than explicitly reference it here, especially if we aren't the ones maintaining it. It'll also be quite wasteful to clone this repo just for the yaml only.

[erictraut]

I've updated the documentation accordingly.