Add doc readme for tying version to pre-commit

[ewjoachim]

Related to #9

Added a doc section for listing recipes for tying the version with a pyright version specified elsewhere, added a first recipe for pre-commit.

Would have happily added poetry.lock but it seems not to be easily yq-parsable.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ab0f126) to head (471738f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #98   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          590       590           
  Branches       118       140   +22     
=========================================
  Hits           590       590           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jakebailey]

This is sort of a meta question, but do people commonly pin pyright only via pre-commit? And not via dev-requirements or some other similar functionality provided by a package manager? How do people run it locally? I guess still within pre-commit somehow?

[ewjoachim]

Yep, I tend to run it via pre-commit (pre-commit run --all-files). Additionally, it runs on my local vscode (I don't think there's a good way to tie those 2 together, but pre-commit has the last word because it runs when I commit)

pre-commit is making huge efforts to be extremely easily cachable (which lets their pre-commit.ci check usually run in <5s, an impressive feat when GHA usually hasn't even booted a VM in 5s)(also of course, whatever runs when you commit must be fast otherwise it's very annoying), so it's mandatory that all versions numbers used appear in .pre-commit-config.yaml (you can't specify latest or * or such). That's a bit of a pain, but I think what the tool is bringing enough good to let it have its way.

I tend to use a pre-commit hook (https://github.com/floatingpurr/sync_with_poetry) that will ensure the versions defined in .pre-commit-config.yaml match the corresponding software versions in poetry.lock, in which case, poetry becomes the source of truth

Note: it's kind of annoying running pyright (or mypy) in pre-commit because each pre-commit tool runs in its own venv which doesn't contain the project dependencies. Because all static checkers need to know your dependencies, you often have to add an explicit list of your deps in .pre-commit-config.yaml which, themselves, should also be synced with poetry.

I think the ideal stack is not there yet. I like what pre-commit brings to the table, even if I don't agree with all the choices.

[ewjoachim]

Note: it's kind of annoying running pyright (or mypy) in pre-commit because each pre-commit tool runs in its own venv which doesn't contain the project dependencies. Because all static checkers need to know your dependencies, you often have to add an explicit list of your deps in .pre-commit-config.yaml which, themselves, should also be synced with poetry.

I think the ideal stack is not there yet. I like what pre-commit brings to the table, even if I don't agree with all the choices.

I've scratched my own itch. There now exists a pre-commit hook that binds the additional dependencies of a hook to a set of poetry groups. This way, each software version is either defined once in the repo or synchronized in the CI.

[jakebailey]

Sorry for the delay on properly reviewing and merging this; I've been busy and I wanted to come up with other sections to add for other possible sources for the version.

Given the above, are you still wanting this to be documented like is in the PR as it is?

[ewjoachim]

Given the above, are you still wanting this to be documented like is in the PR as it is?

I'd say syncing Poetry & Pre-Commit is independent from syncing pre-commit and GHA, so I'd say it's still relevant ?

[ewjoachim]

I did the one for poetry.lock:

on: push

jobs:
  pyright:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions-rust-lang/setup-rust-toolchain@v1  # optionnal, just for speedup
      - name: Extract pyright version from pre-commit
        id: pre-commit-pyright-version
        run: |
          cargo install toml2json
          toml2json poetry.lock \
            | jq -r '.package[] | select(.name == "pyright") | "pyright-version=" + .version' \
            >> $GITHUB_OUTPUT

      - uses: jakebailey/pyright-action@v2
        with:
          version: ${{ steps.pre-commit-pyright-version.outputs.pyright-version }}

reading toml with yq doesn't work so I'm installing toml2json from the rust world. Once caches are in place, this takes just a few seconds. The setup-rust-toolchain enables caching, but the image already has cargo, so it can be removed but that makes the thing run in ~20 seconds longer.

See this repo for details (can't promise it will stay forever)
https://github.com/ewjoachim/pyright-poetry/actions/runs/8590615615/job/23538377904

From looking at the pdm lockfile format, I think the same code would work (apart from changing the filename of course). I haven't tried.

For requirements.txt, it would be ideal if requirement-parser supported an entrypoint, but given it doesn't I think the best course of action would be to write a small python script with a pipx hashbang. I'll try to remember providing an example soon.

[jakebailey]

Hm, honestly I think in most of these cases, it's more straightforward to just run the package manager and print out a version, rather than cargo installing a whole package or something. Adding this must package-manager specific documentation is starting to feel pretty heavy 😅

[jakebailey]

At some level, it may be simpler for me to just look for the pyright package installed within an activated python environment and use that version as a default, or add a setting to do so.

[ewjoachim]

Hm, honestly I think in most of these cases, it's more straightforward to just run the package manager and print out a version, rather than cargo installing a whole package or something. Adding this must package-manager specific documentation is starting to feel pretty heavy 😅

Maybe, but I'm not sure all package manager allow getting a version number without parsing human-readable output.
Case in point:

• Poetry's output if for human. You can force-remove colors with --no-ansi (though it's automatic if it detects stdout is not a tty) but then you need to tiptoe around spaces:

$ poetry --no-ansi show pyright
 name         : pyright
 version      : 1.1.357
 description  : Command line wrapper for pyright

dependencies
 - nodeenv >=1.6.0

$ poetry --no-ansi show pyright | grep ' version' | cut '-d:' -f2 | xargs
1.1.357

Note: the ' version' because the line starts with a space and I want to limit the risk of matching another line that contains version, to the point we should probably match on : as well, and note xargs as a quick and dirty way to remove the leading space.

Personally, I really don't like this line. I find it ugly and I'm annoyed that it will break when Poetry maintainers decide to change the human-readable format of the output. In the end, it would be exactly as easy to read the version from the lockfile directly with grep, ignoring that it's a toml file, but I really don't like that for the same reasons.

You could say that it's a problem with poetry not providing a machine-readable output, and/or GHA not providing a builtin tool to query toml files, and/or yq not providing full toml support.

You could also write a small python script that interacts with poetry as a lib, but it's undocumented.

Also, what about requirements.txt ? there's no package manager to ask in this case (well, maybe pip-compile and pip, but neither can output the info simply).

I think if you want to go this way, the only standard path I see is to say "if pyright is already installed when the action is launched, then we'll use the same version of pyright as installed" [EDIT: ah that's what you suggested]. But it means we'd install pyright twice, the first time for nohing
Clearly, this is a hard problem, but I think it's a problem that needs solving :) Happy to help bouncing ideas if this helps.

Just to make sure I had understood correctly, and though I'm perfectly ok with my contribution not being merged at the moment, but you said:

As more projects keep pinning pyright using different mechanisms (package.json, requirements.txt, pyproject, precommit), I'm less included to try and include something here just becuase it sounds like a slippery slope of every format that could exist.
I have no problem with providing examples in the README using the different methods, though.

...and I understand you changed your mind on that. Did you have in mind that the way to extract locked version from different package managers would be more straightforward in general?

if it helps, my suggestion of using a rust crate was just because it happened to exist, but we could do a python script too.

[ewjoachim]

(just because I had it around, and in case we change our minds, here's the requirements version)

# .github/get_pyright_version.py

# /// script
# dependencies = ["requirements-parser", "setuptools"]
# ///

import requirements


def get_pyright_version():
    with open('requirements.txt') as f:
        reqs = list(requirements.parse(f))
    for req in reqs:
        if req.name == 'pyright':
            return req.specs[0][1]
    raise ValueError('pyright not found in requirements.txt')

if __name__ == '__main__':
    print(get_pyright_version())

$ pipx run --python=python3.10  .github/get_pyright_version.py
1.1.357

[jakebailey]

...and I understand you changed your mind on that. Did you have in mind that the way to extract locked version from different package managers would be more straightforward in general?

I think I expected getting info out of the various places to be as simple a one-liner as I've used for package.json, so seeing an entire cargo install sort of put me off.

we could do a python script too.

The most consistent way on GitHub Actions is probably going to be to use npm, given actions themselves are in Node (so it'll always be working). But, that's not really a big deal.

I think if you want to go this way, the only standard path I see is to say "if pyright is already installed when the action is launched, then we'll use the same version of pyright as installed" [EDIT: ah that's what you suggested]. But it means we'd install pyright twice, the first time for nohing

I don't think that method would require two installs; I can just look on PATH for pyright, then run it like I do node + a downloaded npm package. All the action does is execute a subprocess and collect its output. Realistically, most users of pyright are already going to have to have installed their environment and activated it anyway, so this wouldn't be unheard of, and an opt-in would allow it to not surprisingly appear for those who assume it'd get the latest.

That being said, your original comment was that you wanted to have this work for pre-commit. Is pre-commit already downloading pyright and putting it on PATH such that this would work?

Apologies for waffling back and forth on this; I'm just trying to make sure that a final solution covers the most uses cases without being too complicated. I hadn't thought of "use the version on PATH" before (or at least, I probably forgot about it, or didnt't think of a way to make it not confusing.)

[ewjoachim]

I think I expected getting info out of the various places to be as simple a one-liner as I've used for package.json, so seeing an entire cargo install sort of put me off.

I understand, and once again, it's not the only way.

The most consistent way on GitHub Actions is probably going to be to use npm, given actions themselves are in Node (so it'll always be working). But, that's not really a big deal.

Wait, are we talking about changing the action or documenting it ? In the action, I agree that Node is best. But for the end user, they have equal access to anything that's pre-installed on the VM.

That being said, your original comment was that you wanted to have this work for pre-commit. Is pre-commit already downloading pyright and putting it on PATH such that this would work?

Nope, this solution won't work with pre-commit, but what I currently have works and will continue to work. My original goal was just to document it, to share with others.

Apologies for waffling back and forth on this; I'm just trying to make sure that a final solution covers the most uses cases without being too complicated. I hadn't thought of "use the version on PATH" before (or at least, I probably forgot about it, or didnt't think of a way to make it not confusing.)

No worries, no need to apologize, I just wanted to make sure I had understood your opinion properly.

Again, I don't need anything more for my case to work.

[jakebailey]

In #107 I'm adding version: PATH; this comes with a new readme section specifically about sourcing pyright versions, which I think this PR should be able to merge into pretty cleanly. I think this means we don't need any of the parsing stuff for general package managers, but the pre-commit one is sufficiently different that it's still good to add.

[ewjoachim]

(do you want me to make another pass or would you rather do it ?)

[jakebailey]

Feel free, I'm pretty swamped so you'd probably beat be to it.