NLeSC / python-template

Netherlands eScience Center Python Template
https://research-software-directory.org/software/nlesc-python-template
Apache License 2.0
163 stars 73 forks source link

use ruff instead of prospector and isort (Refs #336) #347

Closed cwmeijer closed 9 months ago

cwmeijer commented 10 months ago

Description

I replace every use or mention of 'prospector' or 'isort' with a 'ruff' equivalent. See discussion in #336. I don't think the linting will be exactly the same as it was, but it will be good and much faster. Same goes for sorting imports.

Instructions to review the pull request

Create a python-template-test repo on GitHub (will be overwritten if existing)

cd $(mktemp -d --tmpdir py-tmpl-XXXXXX)
cookiecutter -c <pr-branch> https://github.com/<pr-user>/python-template
# Fill with python-template-test info
cd python-template-test
git init
git add --all
git commit -m "First commit"
git remote add origin https://github.com/<you>/python-template-test
git push -u origin main -f
python -m venv env
source env/bin/activate
python -m pip install --upgrade pip setuptools
python -m pip install '.[dev,publishing]'

# Some things you could try to test it:
ruff .
ruff . --fix
pre-commit  #after enabling 
pytest .\tests\test_project.py::test_ruff_check
cwmeijer commented 10 months ago

Not sure why the tests are seem to be failing on python versions 3.9 and above. I'll check later. Locally, on python 3.11, I get the following. image

egpbos commented 10 months ago

On my machine/setup (macOS, Python 3.9.13) tests pass on the main branch, so it's probably not some package update...

egpbos commented 10 months ago

Ah, no, that was not the template test but the test from the generated package...

egpbos commented 10 months ago

Ok, it does seem to be some kind of update, because the failures happen on main now as well: https://github.com/egpbos/python-template/actions/runs/6106092589

egpbos commented 10 months ago

The problem originates with this Sphinx 7.2 functionality which was added since August: https://www.sphinx-doc.org/en/master/usage/extensions/coverage.html#confval-coverage_statistics_to_report

No idea why it doesn't generate this in 3.7 and 3.8...

LourensVeen commented 10 months ago

Maybe the new version is marked as requiring 3.9 or higher, so that if you pip install Sphinx on 3.7 or 3.8 you get an older version that doesn't do this?

egpbos commented 10 months ago

@LourensVeen you nailed it! https://github.com/sphinx-doc/sphinx/blob/fcc38997f1d9b728bb4ffc64fc362c7763a4ee25/pyproject.toml#L16C26-L16C26

Ok, so I would say this is a separate issue (i.e. we don't have to block this PR over it), but just to start the discussion: how should we handle this? Just skip the test_coverage_api_docs test for Python < 3.9? Would be as easy as adding a line like this: https://docs.pytest.org/en/7.1.x/how-to/skipping.html#id1, so maybe it could actually be done in this PR just to keep things simple.

cwmeijer commented 10 months ago

I updated the test in line with @egpbos 's comment. This means we are not testing for documentation coverage at all, for versions before 3.9, which is fine I think. For reasons, I created a separate PR which I merged into this one myself. Ask me about it at the water cooler ;-)

Of course, writing a test was more complicated than expected, as lines in the report switch order between runs. image image

bouweandela commented 10 months ago

Does it really take almost 10 minutes to run the tests on a project with barely any content?

cwmeijer commented 10 months ago

@bouweandela, On Ubuntu about 3-4 minutes and 8-9 minutes for mac or windows. While running the tests, about 9 times, a fresh virtual environment is created, in which a new package is generated and installed using the cookie cutter template.

I have to say that the tests are actually quite useful and already helped me find errors a couple of times, which I would probably have overlooked otherwise.

Note that this PR is about reducing the computation time needed for linting the generated package. It hardly reduces the testing time of the template (this repo).

egpbos commented 10 months ago

10 minutes sounds normal, unfortunately. Installing dependencies is slow.

bouweandela commented 9 months ago

@bouweandela, On Ubuntu about 3-4 minutes and 8-9 minutes for mac or windows. While running the tests, about 9 times, a fresh virtual environment is created, in which a new package is generated and installed using the cookie cutter template.

I had a go at making that a bit faster in #353

egpbos commented 9 months ago

I'm going to rebase this branch slightly as well.

Just for your entertainment, here is what the branch looked like before:

* 54fb005 minor fixes for adding prospector (HEAD -> 336-use-ruff-instead-of-prospector-and-isort, upstream/336-use-ruff-i>
|
*   b936ef9 Merge pull request #349 from NLeSC/fix-coverage-api-docs
|\
| |
| * ab72989 update test to expect the output of a new version of sphinx See https://github.com/NLeSC/python-template/pull/>
|/
|
* 2772376 add ruff test and remove isort and prospector tests (Refs #336))
|
* 1b48732 bring template code and ruff config in sync (Refs #336)
|
* c36c520 use ruff instead of prospector and isort (Refs #336)
|
*   a53c275 ...

Of course, that little bump over there is not UGH 😆 Also, my commit can be a squashed together with the first three commits, all four are part of one functional change. The fixed test is functionally different, so we can keep that one separate. Although, for clarity we could keep the 1b48732 bring template code and ruff config in sync commit separate for the latter reason as well; ruff checks are apparently slightly different, but it would not be immediately clear just from looking at the code changes in a combined commit that for instance those docstring changes were actual functional changes instead of just cosmetic (which often is done together with functional changes and which then often confuses some reviewers 😄 ). So, we do:

git rebase -i HEAD~5

And we rewrite history into:

reword ab72989 update test to expect the output of a new version of sphinx See https://github.com/NLeSC/python-template/     pull/347#issuecomment-1710684574
pick c36c520 use ruff instead of prospector and isort (Refs #336)
squash 2772376 add ruff test and remove isort and prospector tests (Refs #336))
squash 54fb005 minor fixes for adding prospector
pick 1b48732 bring template code and ruff config in sync (Refs #336)

Note that just for fun, I also put the fixed test first and reworded it so that the "See URL" part is in the message instead of the title (there was no newline in between so that git saw it as one title). UGH!

egpbos commented 9 months ago

Thanks a lot for this!