ICB-DCM / pyPESTO

python Parameter EStimation TOolbox
https://pypesto.readthedocs.io
BSD 3-Clause "New" or "Revised" License
216 stars 47 forks source link

Code checks not accurately reflects the quality of code #1304

Closed PaulJonasJost closed 4 months ago

PaulJonasJost commented 7 months ago

We are currently running only the pre-commit hook in the tests, i.e. only the changed files will be tested. In this case new updates on quality checks will only be caught when they are actually changed in the code. Two proposed solutions

  1. Run the pre-commit hook in the CI on all files
  2. Run the pre-commit hook on PRs to main on all files

First option is easier to implement and guarantees code style in develop as well. Might require more time for testing though. What do you guys think @ICB-DCM/pypesto-maintainers @dilpath @dweindl

dilpath commented 7 months ago

There is already this action [1], which runs on Mondays and Thursdays currently [2], that shows the current main fails the quality checks. It might make sense to change this CI to run on develop instead -- that main complies with code quality standards should only be ensured when releases occur.

Option (2) makes sense to me, otherwise the action in [1] will fail often, and Option (2) ensures that pyPESTO versions comply with contemporary code quality standards. Option (1) (assuming this applies to all PRs to develop) makes less sense to me, since PRs to develop are often not about maintaining pyPESTO as a whole.

[1] https://github.com/ICB-DCM/pyPESTO/actions/runs/7779288835 [2] https://github.com/ICB-DCM/pyPESTO/blob/b4a187f8ac10ce0cc12a22f49cd75325f3811d8c/.github/workflows/ci.yml#L12

PaulJonasJost commented 7 months ago

Mhh, so I checked again the CI and theoretical is says in the setup of the quality test that is running all files, though we for example did not catch a B038 and at least as far as I could see this was very old and the update introducing this is from 3 weeks ago. I assumed it was only checking changed files to not catch that one 🤔

dilpath commented 7 months ago

It could be a caching issue -- maybe the latest versions of quality checks are not installed every time https://github.com/ICB-DCM/pyPESTO/blob/b4a187f8ac10ce0cc12a22f49cd75325f3811d8c/.github/workflows/ci.yml#L344-L350

dweindl commented 7 months ago

Run the pre-commit hook in the CI on all files

:+1:

Yes, the previous issues were due to caching. We can disable caching for the quality-check, setup is rather quick there.

Furthermore, I'd switch to switch to ruff instead of running five different tools. It's also much faster than flake8.

PaulJonasJost commented 7 months ago

I will look into it and change to ruff, thanks Daniel.