DEV: Add name-tests-test to pre-commit hooks #23213
Conversation
|
@greglucas Given that we discussed this on Gitter, this is ready for review as pre-commit.ci is passing. Also $ pre-commit run name-tests-test --all-files
python tests naming......................................................PassedObviously no rush here, and I'm very happy to explain any questions that anyone might have. 👍 |
|
If we are going to keep adding to pre-commit, then we need to ask everyone to run pre-commit locally before pushing to CI, which I don't believe we currently do in the dev guide. |
@jklymak I thought that part of the resolution from the discussions starting at #22777 (comment) (most of it linked up for reference here in Issue #22949) was that having pre-commmit.ci turned on wouldn't require everyone to use pre-commit locally if they didn't want to as it would fail out (quickly) with decently explicit logs. While I'm coming from the cultural view that wanting people to use pre-commi 8000 t locally is a good idea, I believe that the matplotlib dev team consensus was that pre-commit should not be a required part of the dev environment to avoid any barriers to contribution (thought I can't find a reference for this at the moment easily, so I'll defer to you here). So, while I certainly don't mind editing the dev guide to recommend this, I believe this isn't/wasn't desired. Or are you just suggesting that pre-commit be listed as an optional but recommended dependency in the Additional dependencies for development section of the dev guide (it currently is not)? |
|
Should one make running the rest of the tests dependent on pre-commit passing? |
While it is possible with GitHub Actions to depend on the completion of another workflow with the The additional complexity that it would add to the CI maintenance is also maybe not worth it when you could just throw in a |
|
No, I do not know either. Not even sure if one can make it depend on e.g. linting which is in a separate file (without moving it to the same). I think it cancels now (as I quite often to follow-up pushes and try to get quite a few emails that something was cancelled), but if there is a better way one should probably look into that (and confirm that it does cancel now). |
Ah yeah, you're right. matplotlib/.github/workflows/tests.yml Lines 2 to 4 in e6e63b4 got added in PR #22919 (:rocket:) which is great. |
23cd6d4 to
547819d
Compare
|
@jklymak gentle ping to see if you have follow up thoughts/clarification on recommendations on pre-commit checks. |
|
Sorry, I'm neutral on this - but I did install the hooks locally so I stop pushing bad commits 😄 |
Cool. :) Do you have anything that you'd like to see added to this PR then?
👍 I hope that you are finding the experience okay and hopefully nice. If you have feedback on your experience of installing the hooks and getting going I would be very happy to try to incorporate that as suggested recommendations into the developer docs if that's viewed as helpful. |
|
Installing it was fine. Its just one more thing to install, that's all, and I work on different machines, so I tend to be resistant to too many "extras" |
2561ef3 to
00983ba
Compare
* Add the 'name-tests-test' pre-commit hook which verifies that test files under lib/matplotlib/tests/ conform to pytest naming conventions. - Use the '--django' option to match the 'test*.py' naming convention.
00983ba to
ade569e
Compare
PR Summary
Add the 'name-tests-test' pre-commit hook which verifies that test files under
lib/matplotlib/tests/conform topytestnaming conventions. Use the--djangooption to match thetest*.pynaming convention.PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).