Example of it running when creating this PR:

It's also easy to run this automatically on PRs usinghttps://pre-commit.ci/ which I like doing

There is some conversation going on about detecting added and then removed large files during a PR#21593. Adding the check-added-large-files would prevent me from committing locally, but I'm curious if using the pre-commit.ci would find that file or not? I think another way of phrasing that is: if I push a PR with 20 commits, does pre-commit.ci run on the squashed version, or does it run on every single commit?

If anyone is inspired to experiment pre-commit.ci is installed onhttps://github.com/matplotlib/ipympl so a PR there could be used to experiment.

Can you even run pre-commit install for users automatically through the conda env create command somehow?

I have no idea, but that seems a bit invasive unless there was an easy opt out (like a flag in the create command). Though it seems that conda allowing that would be a security risk of some sort so I don't expect it to be possible)

I haven't used it before, but I am curious about the auto-fixing whitespace and things for new contributors, as that may make it less intimidating to contribute to the codebase.

I think this is a really nice benefit, not having to mess around with lint errors. Though the conflicts are definitely an issue, they will either need topull immediately or force push and communicating that could be tricky - but maybe accomplished with a commenting bot?

So for autofixing we can turn off the autofix functionality of pre-commit.ci usinghttps://pre-commit.ci/#configuration-autofix_prs so it would not push commits, only fail (also looks like you can exclude certain checks) but they would still run locally.

dding the check-added-large-files would prevent me from committing locally, but I'm curious if using the pre-commit.ci would find that file or not? I think another way of phrasing that is: if I push a PR with 20 commits, does pre-commit.ci run on the squashed version, or does it run on every single commit?

Frommatplotlib/ipympl#392 it looks like it unfortunately runs on the squashed version

Thanks for checking on that! So, it looks like we will need that other duplicate file checker PR anyways. And now that I think about it, this test will only check large files, it wouldn't get flagged for a 400kb file added and then removed, which we also don't want to creep in.

For the CI portion, it looks like the CI will only "fix" the PRs? It won't give a comment about why it failed, or does it do that as well? I wonder if we want to have a separate GitHub Actions CI and just run pre-commit on an instance ourselves.
https://pre-commit.com/#github-actions-example
with a--show-diff-on-failure to try and make it easy to point out to new contributors. We could also add in afail_fast: true to save CI resources if the pre-commit job fails immediately.

All these comments can be added in a follow-up PR too, as they may require more discussion than just adding in the config file and instructions.

For the CI portion, it looks like the CI will only "fix" the PRs? It won't give a comment about why it failed, or does it do that as well?

I set it so that it won't push commits to PRs unless asked to. It should still run and thedetails will give a reason for failure. For example see this PR:mpl-extensions/mpl-image-labeller#18 (although that has pushing commits on, but the final commit still fails pre-commit as it doesn't autofix everything)

After clicking through the errors look like this:

Compared to the current linting workflow on failure:https://github.com/matplotlib/matplotlib/runs/4159189797?check_suite_focus=true

When running in ci I believe it will check all files in the repo, not just those changed in the PR (locally it just checks what is being committed). So if this going to be set up in CI we need to get to all green onpre-commit run --all-files. To that end I've added excludes and will push a commit with autofixes

Currently runningpre-commit run --all-files gives:

Check for added large files..............................................PassedCheck docstring is first.................................................Failed- hook id: check-docstring-first- exit code: 1examples/images_contours_and_fields/colormap_normalizations_symlognorm.py:15 Multiple module docstrings (first docstring on line 1).lib/matplotlib/dates.py:213 Multiple module docstrings (first docstring on line 1).lib/matplotlib/_constrained_layout.py:27 Multiple module docstrings (first docstring on line 1).lib/matplotlib/backend_tools.py:992 Multiple module docstrings (first docstring on line 1).Fix End of Files.........................................................Failed- hook id: end-of-file-fixer- exit code: 1- files were modified by this hookFixing plot_types/basic/README.rstFixing plot_types/stats/README.rstFixing LICENSE/LICENSE_BAKOMAFixing .github/workflows/clean_pr.ymlFixing doc/api/lines_api.rstFixing .github/ISSUE_TEMPLATE/feature_request.ymlFixing plot_types/arrays/README.rstFixing doc/api/transformations.rstFixing LICENSE/LICENSE_STIXFixing doc/devel/testing.rstFixing doc/_static/.gitignoreFixing LICENSE/LICENSE_COURIERTENFixing lib/matplotlib/tests/READMEFixing lib/matplotlib/backends/web_backend/css/page.cssFixing plot_types/unstructured/README.rstFixing plot_types/README.rstFixing doc/devel/style_guide.rstFixing LICENSE/LICENSEFixing LICENSE/LICENSE_CARLOGOFixing doc/api/toolkits/mplot3d/faq.rstFixing README.rstFixing doc/api/_enums_api.rstMixed line ending........................................................PassedTrim Trailing Whitespace.................................................Failed- hook id: trailing-whitespace- exit code: 1- files were modified by this hookFixing doc/users/faq/index.rstFixing doc/users/index.rstFixing doc/api/next_api_changes/removals/00001-ABC.rstFixing doc/users/getting_started/index.rstFixing lib/matplotlib/backends/web_backend/css/fbm.cssFixing doc/README.txtFixing doc/api/patches_api.rstFixing LICENSE/LICENSE_BAKOMAFixing doc/api/tri_api.rstFixing doc/api/lines_api.rstFixing .github/ISSUE_TEMPLATE/feature_request.ymlFixing .github/ISSUE_TEMPLATE/documentation.ymlFixing doc/devel/contributing.rstFixing doc/users/next_whats_new/extending_MarkerStyle.rstFixing LICENSE/LICENSE_STIXFixing doc/api/next_api_changes/behavior/00001-ABC.rstFixing .github/ISSUE_TEMPLATE/maintenance.ymlFixing examples/event_handling/README.txtFixing CODE_OF_CONDUCT.mdFixing doc/api/next_api_changes/development/00001-ABC.rstFixing LICENSE/LICENSE_AMSFONTSFixing doc/_templates/automodule.rstFixing doc/devel/documenting_mpl.rstFixing plot_types/unstructured/README.rstFixing LICENSE/LICENSE_COLORBREWERFixing .github/ISSUE_TEMPLATE/bug_report.ymlFixing plot_types/README.rstFixing doc/users/explain/fonts.rstFixing src/_path.hFixing doc/users/resources/index.rstFixing SECURITY.mdFixing src/_backend_agg.hflake8...................................................................Passed

With the last two commits the only failure is:

Check docstring is first.................................................Failed- hook id: check-docstring-first- exit code: 1lib/matplotlib/_constrained_layout.py:27 Multiple module docstrings (first docstring on line 1).

The other docstring first failures were easy fixes but this one I didn't feel comfortable changing.

@jklymak do you have thoughts on what to do there?

This gained many files once I tested running pre-commit to get it to a place where everything passes. Happy to drop that commit and/or squash down as seems reasonable

I'd like to suggest that if pre-commit.ci is considered being added as an integration it is added before this is merged so that everyone can get a feel for it while it is only affecting this PR.

The other docstring first failures were easy fixes but this one I didn't feel comfortable changing.

I am not quite following -_contrained_layout.py doesn't fail our usual flake8 - what are you doing here that is different?

The other docstring first failures were easy fixes but this one I didn't feel comfortable changing.

I am not quite following - _contrained_layout.py doesn't fail our usual flake8 - what are you doing here that is different?

This is failing thecheck-docstring-first check because there is a second module level docstring in that file here:

Sure I appreciate that. However I'm not sure that we want to start being more stringent than our current linting. Can this check be implemented without changing our current policy? because our linting policy is a larger conversation.

I actually think all of the linting updates look good, it isn't black formatting things ;) For the double docstring I'd just combine it all into one or turn the second one into a massive comment. Since it is a private module, I don't think it gets rendered anywhere in the docs...

I also agree that it would be helpful to separate that commit out into a separate PR. So, turn this into two PRs. A new one with a lot of linting updates, and the other with just the pre-commit configs.

OK, I see, triple quotes are not to be used forin-line comments (I didn't know that before!). I guess I'm fine with that if it is a bonafide python standard, and I'm fine with the triple quotes in_constrained_layout.py being coverted to#.

I'm going to block on all the linting changes. If this pr can go in without them, then great I'm totally fine with the convenience.

I also agree that it would be helpful to separate that commit out into a separate PR. So, turn this into two PRs. A new one with a lot of linting updates, and the other with just the pre-commit configs.

Sure thing. I added all the changes here because I was concerned about adding an automated check that would fail on some files (or on any CI check) so if this does go in I think it would be nice to also have the linting one go in soon after.

An alternative PR breakup strategy that preserves the always passing of the check could maybe be as follows:
PR 1: This one minus the docstring check (which I think would be the only contentious styling check)
- config commits squashed into one for easy review
PR 2: Adding in potentially more checks (e.g. docstring) and having a longer discussion of lint rules.

But for now (likely tomorrow or monday) I'll break it up as suggested one for config, and one for changes

This PR is now just the config and#21638 is config plus linting changes.

OK, I see, triple quotes are not to be used for in-line comments (I didn't know that before!). I guess I'm fine with that if it is a bonafide python standard, and I'm fine with the triple quotes in _constrained_layout.py being coverted to #.

I'm not sure that they are "not to be used" more so that you're (mis?)using a language features of defining a multiline string and not assigning it to a variable.

My approval still stands here.

It looks like we just need to authorizepre-commit.ci on thematplotlib/matplotlib repository if we want to have the CI checks and it appears any maintainer can do that. We should discuss at a dev call whether we want to enable the service generally or not. For now, adding a configuration file shouldn't affect any users unless they are using their own local pre-commit configuration already, so I still think this is a plus and then I can take advantage of it ;)

@ianhi, do you know if you remove theci: ... from the pre-commit config will that disable the CI checks, or is the only way to turn on/off thepre-commit.ci service to enable/disable on the repository?

@ianhi, do you know if you remove the ci: ... from the pre-commit config will that disable the CI checks, or is the only way to turn on/off the pre-commit.ci service to enable/disable on the repository?

I'm a little confused exactly what your asking. Theci: ... is currently preventing the CI service pushing commits to PRs but I believe it would still run the checks and give a pass/fail. To turn off the check you need to (dis/en)able on the whole repo.

To turn off the check you need to (dis/en)able on the whole repo.

👍 that is exactly what I was wondering, thanks!

@ianhi, I think there was consensus to not usepre-commit.ci for now, since we aren't auto-fixing PRs. Instead, we should replace the current flake8 runner underworkflows/reviewdog.yml with the pre-commit hook checks instead (piping the output from pre-commit into reviewdog). Something like:pre-commit run --from-ref origin/HEAD --to-ref HEAD | reviewdog. (I'm not sure if that will work directly because it shows the status of each hook when run, so perhaps there is a better command line option to only get the failure text to pipe into reviewdog)

It looks like you can also cache the pre-commit environment for use next time too:https://pre-commit.com/#github-actions-example

@ianhi, I think there was consensus to not use pre-commit.ci for now, since we aren't auto-fixing PRs.

Sure thing, though note I'm pretty sure that with the config in this PR even pre-commit.ci willnot push autofixes, it would merely report which checks failed and should (I think) be faster than github actions.

Right, the thinking was that instead of adding "yet another service", we could get the benefits of using pre-commit just replacing running flake8 in our existing GitHub Actions job with pre-commit. In my experience, the speed of the linting job isn't really important (compared with say tests/docs).

@tacaswell was that an accidental merge? per#21638 (comment) we may sitll need to remove the "trailing-whitespace" fixer?

Ah, the changes were gone from the diff so I thought it was dealt with.

I was not a fan of seeing all of the thrashing now, but if it comes in as we actually touch those files that seems more OK to me. The worst offenders are the vendored copy of Agg (which we basically never touch so the hooks will not see them, and if we do it will be by someone who knows how to turn them off ;) ).

I can see the case for adding rst to the ignored files on the trailing whitespace, but lets see how much we actually run into that first?