openzim / _python-bootstrap

Sample openZIM Python project bootstrap
1 stars 1 forks source link

pyright is missing from optional dependencies + wrong `check` env dependencies #6

Closed benoit74 closed 11 months ago

benoit74 commented 11 months ago

In pyproject.toml, pyright is not present as an optional dependencies.

And hatch check environment depends from test optional dependencies which it does not use.

rgaudin commented 11 months ago

It's on purpose. Pyright is a node tool. That python package is just a wrapper that installs it and walk over your PATH to find it. While I think the intention is good, I want to control where this tool is going to be installed and when to upgrade. It can be done with the wrapper but it requires setting environment variables… which is outside the scope of our python setup and thus cancels the benefit of it.

Without the wrapper

With the wrapper

As written above, it's possible to control where to install, but dev has to set some wrapper-specific environment variables and make sure those are exposed for every pyright call.

I find the no-wrapper approach cleaner and more flexible but I agree it introduce an initial fail. If that's a concern, we could react to it missing in the invoke tasks and display a dev-friendly message leading to the install documentation.

What do you think?

rgaudin commented 11 months ago

We could also have the wrapper in a feature and advertise it should pyright call failed due to missing binary.

benoit74 commented 11 months ago

I understand most of your concerns, but (yes, there is a but 🤣)

pyright 1.1.312

- the wrapper works well when offline, timeout is set at 1 sec (obviously two stuff are tried since it took 2 secs in my test)

time pyright 0 errors, 0 warnings, 0 informations pyright 2.02s user 0.15s system 146% cpu 1.474 total

This first point is a significant concern for me, as a developer I usually prefer to not mind about such details, and as a maintainer I would prefer to not have to explain this to every new developer on a project.

And yes, I added the wrapper in a feature in my PR.

rgaudin commented 11 months ago

OK I'm sold 😉 Thanks for the details. I clearly misread the README.