search

rstudio / pins-python

https://rstudio.github.io/pins-python/
MIT License
49 stars 12 forks source link

Add `rdata` as a test dependency #262

Open nathanjmcdougall opened 1 month ago

nathanjmcdougall commented 1 month ago

At the moment, rdata is similar to pyarrow, etc. in the sense that it is optional, but still needed for testing and so I think it should be explicitly declared as a optional test dependency in setup.cfg.

This will mean it can be added as a pinned version to requirements/dev.txt rather than installed separately. This ensures consistency between local dev and CI and also ensures there is a consistent dependency resolution with dev.txt together with the rdata package.

I would like to do this, then the separate install step could be removed: https://github.com/rstudio/pins-python/blob/70380b4532f3778e4777888507a49c2d1cab1ae8/.github/workflows/ci.yml#L90-L95

Incidentally, I am not sure why python -m pip install -e .[test] is necessary since the test extra is already included in the dev.txt file. I think this could be removed too.

nathanjmcdougall commented 1 month ago

This is also the only thing keeping us from using reportMissingImports = true in Pyright (see #256).

isabelizimm commented 1 month ago

Thanks for this bit of cleanup, these seem like great ideas to do a bit of maintenance! I would want to make sure that there is at least one set of tests run that does not include all the test dependencies, but that case should be covered by the min test requirements CI run.

since the test extra is already included in the dev.txt file.

You are correct; it should be python -m pip install -e . 👀 pyarrow and fastparquet were not part of dev.txt when that line was added, but we have since updated that requirements file to be comprehensive.