@greglucas Given that wediscussed this on Gitter, this is ready for review as pre-commit.ci is passing. Also

$pre-commit run name-tests-test --all-filespython tests naming......................................................Passed

Obviously 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.

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 fromthe cultural view that wanting people to use pre-commit 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 theAdditional 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?

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 theworkflow_run event I personally don't know how easily/if it can be done for an arbitrary webhook.

The additional complexity that it would add to the CI maintenance is also maybe not worth it when you could just throw in aconcurrency block so that a follow up push (from the correction that pre-commit found) automatically cancels the previous running job (if there is one).

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).

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.

got added in PR#22919 (:rocket:) which is great.

@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 😄

Sorry, I'm neutral on this

Cool. :) Do you have anything that you'd like to see added to this PR then?

but I did install the hooks locally so I stop pushing bad commits smile

👍 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"