Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

cwmeijer
Copy link
Member

@cwmeijer cwmeijer commented Sep 6, 2023

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
Copy link
Member Author

cwmeijer commented Sep 6, 2023

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
Copy link
Member

egpbos commented Sep 7, 2023

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

@egpbos
Copy link
Member

egpbos commented Sep 7, 2023

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

@egpbos
Copy link
Member

egpbos commented Sep 7, 2023

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
Copy link
Member

egpbos commented Sep 7, 2023

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
Copy link
Member

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?

@cwmeijer cwmeijer marked this pull request as ready for review September 7, 2023 09:57
@egpbos
Copy link
Member

egpbos commented Sep 7, 2023

@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
Copy link
Member Author

cwmeijer commented Sep 11, 2023

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
Copy link
Member

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

@cwmeijer
Copy link
Member Author

cwmeijer commented Sep 12, 2023

@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
Copy link
Member

egpbos commented Sep 12, 2023

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

Copy link
Member

@egpbos egpbos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Commented on issues that should be addressed, good to merge after that, I think.

@bouweandela
Copy link
Member

@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
Copy link
Member

egpbos commented Oct 17, 2023

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 egpbos force-pushed the 336-use-ruff-instead-of-prospector-and-isort branch from 54fb005 to e3d1275 Compare October 17, 2023 19:55
@egpbos egpbos merged commit ebc67dd into main Oct 17, 2023
16 checks passed
@egpbos
Copy link
Member

egpbos commented Oct 17, 2023

Thanks a lot for this!

@sjvrijn sjvrijn deleted the 336-use-ruff-instead-of-prospector-and-isort branch May 3, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants