Add pre-commit config and dev instructions #21583
Conversation
|
Example of it running when creating this PR:
|
doc/devel/development_setup.rst
Outdated
| =========================== | ||
| You can optionally install `pre-commit <https://pre-commit.com/>`_ hooks. | ||
| These will automatically check flake8 and other style issues when you run | ||
| `git commit`. To install the hooks :: |
There was a problem hiding this comment.
| `git commit`. To install the hooks :: | |
| ``git commit``. To install the hooks :: |
There was a problem hiding this comment.
Probably should include instructions for where the yaml is in case folks need to tweak it?
.pre-commit-config.yaml
Outdated
| - repo: https://github.com/myint/autoflake | ||
| rev: v1.4 | ||
| hooks: | ||
| - id: autoflake | ||
| args: ["--in-place", "--remove-all-unused-imports", "--ignore-init-module-imports", "--remove-unused-variables"] |
There was a problem hiding this comment.
I don't like auto-corrections that go beyond basic stuff like end-fo-file-fixer or trailing-whitespace. Anyway, one cannot apply the unused import detection as is to the current codebase because we have cases like this:
matplotlib/lib/matplotlib/text.py
Line 16 in c12791c
So let's leave this out.
There was a problem hiding this comment.
Sure thing.
Although note that I think this basically pushes you to to use __all__ or # noqa either of which would prevent that from being removed.
There was a problem hiding this comment.
We currently do that via F401 excludes in .flake8.
The # noqa comment is quite non-instructive. Technically, something like would work for flake8 but has the disadvantage that lines become too long very quickly and you get line-length-complaints. Also, last time I checked, not all tools can work with this expanded syntax.
from .textpath import TextPath # Unused, but imported by others. # noqa: F401
There was a problem hiding this comment.
I don't think autocorrections are a good idea either. That seems likely to sow more confusion than it helps. I don't think new contributors find flake8 intimidating, and many IDEs will show you where the flake8 errors are already, so I don't think its too cumbersome to ask users to edit by hand. If this were togglable, maybe that would be OK, but default off?
There was a problem hiding this comment.
The # noqa comment is quite non-instructive.
Agreed, my preference would still be to use __all__ = ["TextPath", ...] and re-enable F401. This feels like the correct way to specify the "exports" of a file while also allowing checks for actually unused imports.
|
It's also easy to run this automatically on PRs using https://pre-commit.ci/ which I like doing |
There was a problem hiding this comment.
👍, it is a nice-to-have and doesn't force people to use it. Some comments for thought/discussion about expanding this to more things below, but overall I like the idea of adding this.
I'd suggest adding pre-commit to the environment.yml file. Can you even run pre-commit install for users automatically through the conda env create command somehow? That may be getting too invasive, but I feel like it could help users out locally rather than having CI tell them something is wrong and having to search through logs that can be long and cryptic.
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?
For pre-commit.ci, I think running it would be good. 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. However, I'm curious if those users would be able to get those pushed changes from CI back into their local copy without going through merge/rebase issues which may actually be worse and more confusing.
If anyone is inspired to experiment pre-commit.ci is installed on https://github.com/matplotlib/ipympl so a PR there could be used to experiment.
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 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 to |
.pre-commit-config.yaml
Outdated
| - repo: https://github.com/myint/autoflake | ||
| rev: v1.4 | ||
| hooks: | ||
| - id: autoflake | ||
| args: ["--in-place", "--remove-all-unused-imports", "--ignore-init-module-imports", "--remove-unused-variables"] |
There was a problem hiding this comment.
I don't think autocorrections are a good idea either. That seems likely to sow more confusion than it helps. I don't think new contributors find flake8 intimidating, and many IDEs will show you where the flake8 errors are already, so I don't think its too cumbersome to ask users to edit by hand. If this were togglable, maybe that would be OK, but default off?
|
So for autofixing we can turn off the autofix functionality of pre-commit.ci using https://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. |
From matplotlib/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. 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. |
I set it so that it won't push commits to PRs unless asked to. It should still run and the 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 |
|
|
||
| pip install pre-commit | ||
| pre-commit install | ||
|
|
There was a problem hiding this comment.
Can you add a sentence about where the config file is so devs can modify their local version?
There was a problem hiding this comment.
I added something, let me know if that's what you were looking for
|
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 on |
|
Currently running Check for added large files..............................................Passed Check docstring is first.................................................Failed - hook id: check-docstring-first - exit code: 1 examples/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 hook Fixing plot_types/basic/README.rst Fixing plot_types/stats/README.rst Fixing LICENSE/LICENSE_BAKOMA Fixing .github/workflows/clean_pr.yml Fixing doc/api/lines_api.rst Fixing .github/ISSUE_TEMPLATE/feature_request.yml Fixing plot_types/arrays/README.rst Fixing doc/api/transformations.rst Fixing LICENSE/LICENSE_STIX Fixing doc/devel/testing.rst Fixing doc/_static/.gitignore Fixing LICENSE/LICENSE_COURIERTEN Fixing lib/matplotlib/tests/README Fixing lib/matplotlib/backends/web_backend/css/page.css Fixing plot_types/unstructured/README.rst Fixing plot_types/README.rst Fixing doc/devel/style_guide.rst Fixing LICENSE/LICENSE Fixing LICENSE/LICENSE_CARLOGO Fixing doc/api/toolkits/mplot3d/faq.rst Fixing README.rst Fixing doc/api/_enums_api.rst Mixed line ending........................................................Passed Trim Trailing Whitespace.................................................Failed - hook id: trailing-whitespace - exit code: 1 - files were modified by this hook Fixing doc/users/faq/index.rst Fixing doc/users/index.rst Fixing doc/api/next_api_changes/removals/00001-ABC.rst Fixing doc/users/getting_started/index.rst Fixing lib/matplotlib/backends/web_backend/css/fbm.css Fixing doc/README.txt Fixing doc/api/patches_api.rst Fixing LICENSE/LICENSE_BAKOMA Fixing doc/api/tri_api.rst Fixing doc/api/lines_api.rst Fixing .github/ISSUE_TEMPLATE/feature_request.yml Fixing .github/ISSUE_TEMPLATE/documentation.yml Fixing doc/devel/contributing.rst Fixing doc/users/next_whats_new/extending_MarkerStyle.rst Fixing LICENSE/LICENSE_STIX Fixing doc/api/next_api_changes/behavior/00001-ABC.rst Fixing .github/ISSUE_TEMPLATE/maintenance.yml Fixing examples/event_handling/README.txt Fixing CODE_OF_CONDUCT.md Fixing doc/api/next_api_changes/development/00001-ABC.rst Fixing LICENSE/LICENSE_AMSFONTS Fixing doc/_templates/automodule.rst Fixing doc/devel/documenting_mpl.rst Fixing plot_types/unstructured/README.rst Fixing LICENSE/LICENSE_COLORBREWER Fixing .github/ISSUE_TEMPLATE/bug_report.yml Fixing plot_types/README.rst Fixing doc/users/explain/fonts.rst Fixing src/_path.h Fixing doc/users/resources/index.rst Fixing SECURITY.md Fixing src/_backend_agg.h flake8...................................................................Passed |
|
With the last two commits the only failure is: 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 |
examples/images_contours_and_fields/colormap_normalizations_symlognorm.py
Outdated
Show resolved
Hide resolved
lib/matplotlib/backend_tools.py
Outdated
| @@ -989,12 +989,10 @@ def trigger(self, *args, **kwargs): | |||
| 'help': ToolHelpBase, | |||
| 'copy': ToolCopyToClipboardBase, | |||
| } | |||
| """Default tools""" | |||
There was a problem hiding this comment.
These are docstrings; they should probably be comments instead.
There was a problem hiding this comment.
I thought about converting them but they seemed redundant with eachother and with the variable name. Happy to add back though
There was a problem hiding this comment.
+1 for removing the docstrings are indeed redundant.
|
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. |
I am not quite following - |
This is failing the matplotlib/lib/matplotlib/_constrained_layout.py Lines 27 to 29 in f93c0a3 |
|
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 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 |
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: But for now (likely tomorrow or monday) I'll break it up as suggested one for config, and one for changes |
Update pre-commit hooks rst format Disable autofixes on PRs Unless manually triggered. See https://pre-commit.ci/#configuration add pre-commit to environment.yml Slow down autoupdate schedule alphabetize Add sentence about where pre-commit config is Add excludes to pre-commit config Otherwise end of lines of test images will get fixed, this also ignores all the prev_whats_new and prev_api_changes as well as vendored components (agg, gitwash) pre-comit config
|
This PR is now just the config and #21638 is config plus linting changes. |
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 authorize @ianhi, do you know if you remove the |
I'm a little confused exactly what your asking. The |
👍 that is exactly what I was wondering, thanks! |
|
@ianhi, I think there was consensus to not use It looks like you can also cache the pre-commit environment for use next time too: https://pre-commit.com/#github-actions-example |
Sure thing, though note I'm pretty sure that with the config in this PR even pre-commit.ci will not 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? |
PR Summary
Add a
.pre-commit-config.yamlfile for use with pre-commit as well as add instructions for setting it up. When installed this should shorten the feedback loop for catching flake8 errors as it will run every time you attempt togit commit- it also is smart about running only on files staged for commit.I find this to be very convenient and think it would be nice to include as an option for those who want to use it. But this is certainly something that would be up for discussion.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).