I like the idea! Will this enforce not having any errors? So if one introduce a new abbreviation, it must also go in the ignore list? (I guess it is rare, but just want to understand.)

Seems like not automatically changing is a good idea (not sure what the interface is, but I can see problems with selecting the wrong word etc, so better that it just warns).

I like the idea! Will this enforce not having any errors? So if one introduce a new abbreviation, it must also go in the ignore list? (I guess it is rare, but just want to understand.)

@oscargus No, it is only for character sets that codespell thinks is a mispelling of a common word. For example, if you had a variable named algo things would be fine

diff --git a/lib/matplotlib/artist.py b/lib/matplotlib/artist.py
index 1f33b9d3ec..a301a257f0 100644
--- a/lib/matplotlib/artist.py
+++ b/lib/matplotlib/artist.py
@@ -59,6 +59,7 @@ def allow_rasterization(draw):
                 renderer.stop_rasterizing()
                 renderer.start_rasterizing()
 
+    algo = 1
     draw_wrapper._supports_rasterization = True
     return draw_wrapper

$ pre-commit run codespell --all-files
codespell................................................................Passed

but if you had a variable named mapp it would think you had typoed "map"

diff --git a/lib/matplotlib/artist.py b/lib/matplotlib/artist.py
index 1f33b9d3ec..a543035392 100644
--- a/lib/matplotlib/artist.py
+++ b/lib/matplotlib/artist.py
@@ -59,6 +59,7 @@ def allow_rasterization(draw):
                 renderer.stop_rasterizing()
                 renderer.start_rasterizing()
 
+    mapp = 1
     draw_wrapper._supports_rasterization = True
     return draw_wrapper

$ pre-commit run codespell --all-files
codespell................................................................Failed
- hook id: codespell
- exit code: 65

lib/matplotlib/artist.py:62: mapp ==> map

and then you would need to add mapp to the ignore file if you really wanted this variable name. Snake case is fine though, so it also encourages using it 👍

diff --git a/lib/matplotlib/artist.py b/lib/matplotlib/artist.py
index 1f33b9d3ec..90956c7a4d 100644
--- a/lib/matplotlib/artist.py
+++ b/lib/matplotlib/artist.py
@@ -59,6 +59,7 @@ def allow_rasterization(draw):
                 renderer.stop_rasterizing()
                 renderer.start_rasterizing()
 
+    map_p = 1
     draw_wrapper._supports_rasterization = True
     return draw_wrapper

$ pre-commit run codespell --all-files
codespell................................................................Passed

• 'coo' for COO (Coordinate list)
• 'sur' for Big Sur

Would it fix these by capitalizing them correctly instead? They appear to be comments or docstrings that can easily be fixed.

• 'flate' for Flate compression

I can't find this.

You should also add (if not already automatically ignored by e.g., .gitignore), extern, build, and lib/matplotlib/backends/web_backend/node_modules to be ignored.

'ot' for offset transform

All of these can be written out as a full word, so I've opened #22784 doing so.

• 'coo' for COO (Coordinate list)
• 'sur' for Big Sur

Would it fix these by capitalizing them correctly instead? They appear to be comments or docstrings that can easily be fixed.

@QuLogic Sadly no, as --ignore-words-list is a

comma separated list of words to be ignored by codespell. Words are case sensitive based on how they are written in the dictionary file.

and from codespell-project/codespell#2026 it doesn't really seem like this is possible to have case sensitive ignores. :(

codespell dictionaries are slightly confusingly named as they are lookup of spelling corrections, and not words, passed in with --dictionary which is a

custom dictionary file that contains spelling corrections. If this flag is not specified or equals "-" then the default dictionary is used. This option can be specified multiple times.

So as these aren't typos, but just abbreviations or words that codespell thinks might be typos, there isn't really a good alternative at the moment except to ignore them (as far as I understand).

• 'flate' for Flate compression

I can't find this.

Without it codespell fails with

$ pre-commit run codespell --all-files
codespell................................................................Failed
- hook id: codespell
- exit code: 65

lib/matplotlib/backends/backend_pdf.py:1651: Flate ==> Flat

You should also add (if not already automatically ignored by e.g., .gitignore), extern, build, and lib/matplotlib/backends/web_backend/node_modules to be ignored.

Correct, as this is a pre-commit hook then it only runs over files that are under version control so these don't need to be worried about as you already have them ignored. 👍

Example:

$ echo "mapp" > example.txt
$ pre-commit run codespell --all-files
codespell................................................................Passed

All of these can be written out as a full word, so I've opened #22784 doing so.

Awesome and thank you! I didn't want to presume that things should be modified, so I didn't make any changes in this PR or open a new one to apply changes. Once PR #22784 is merged I'll rebase this one on it. 👍

• 'flate' for Flate compression

I can't find this.

Oh, because it's case-insensitive, okay.

You should also add (if not already automatically ignored by e.g., .gitignore), extern, build, and lib/matplotlib/backends/web_backend/node_modules to be ignored.

Correct, as this is a pre-commit hook then it only runs over files that are under version control so these don't need to be worried about as you already have them ignored. +1

The extern directory is in version control.

The extern directory is in version control.

Ah yeah. whoops! Okay fixed that locally on my fork and I'll push that in the rebase after PR #22784 is in.

edit: I missed this earlier, but the addition of extern/* isn't needed as it is already universally excluded by pre-commit

Another note: Given codespell-project/codespell#2063 I can't add comments to the ignore file to make it clear why these words are ignored. So the commit message and this PR will have to do for the time being.

edit: I've rebased this now with the changes that are needed, so that in the event that I'm not around to have rebased this after PR #22784 is merged a maintainer can rebase it for me and I don't slow down PR review.

I'm always confused by these pre-commit hooks as I don't currently use them. Won't our CI will have to run this as well, otherwise folks who don't use pre-commit hooks will fail folks who do?

But overall, I'm not clear what problem adding this is supposed to fix. No doubt just general ignorance on my part.

I'm always confused by these pre-commit hooks as I don't currently use them. Won't our CI will have to run this as well, otherwise folks who don't use pre-commit hooks will fail folks who do?

Hi @jklymak. 👋 There was back and forth discussion on PR #21583 about enabling pre-commit.ci, and while @ianhi enabled CI running in the config but disabled pushing back fixes (c.f. #21583 (comment))

it apparently never got turned on as

is unknown at the moment.

My take is that pre-commit.ci is well worth the "yet another service" Ryan was unsure about, as it makes it viable for me to actually maintain pre-commit setups across many GitHub organizations, and to make sure that things are actually getting run. As a pre-commit.ci heavy user I'm very happy to discuss this if you have questions.

So yes, to get the full benefit you should run it in CI, but this is easy to do.

But overall, I'm not clear what problem adding this is supposed to fix. No doubt just general ignorance on my part.

pre-commit in general? Or the addition of codespell? If the latter, then automatically catching and notifying you at commit time that you probably have a typo in the English language parts of the code and docs. So typo avoidance for free and one less thing that reviewers need to do. c.f. #22733 (comment) as an example.

No I'm fine with pre-commit as an idea. I just am unsure about codespell. Certainly if it works on our docs and docstrings that is helpful. I was less certain about its use in code per-se.

However it seems we need to get pre-commit to run properly on our CI first?

However it seems we need to get pre-commit to run properly on our CI first?

This just requires a maintainer turning it (https://pre-commit.ci/) on. There's nothing that needs to be done on the GitHub side.

Great - I assume it currently passes? I'll ask on gitter if we can turn it on.

Great - I assume it currently passes?

This PR passes as

$ pre-commit run codespell --all-files
codespell................................................................Passed

but I believe (I could be wrong, as I'm used to having pre-commit.ci run with autofix_prs: true) that pre-commit.ci runs pre-commit run --all-files (as opposed to just pre-commit run which only hits the diff), which currently fails

$ pre-commit run --all-files
Check for added large files..............................................Passed
Check docstring is first.................................................Failed
- hook id: check-docstring-first
- exit code: 1

lib/matplotlib/dates.py:225 Multiple module docstrings (first docstring on line 1).
lib/matplotlib/_constrained_layout.py:28 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/README.rst
Fixing doc/api/transformations.rst
Fixing lib/matplotlib/tests/README
Fixing doc/devel/testing.rst
Fixing doc/users/next_whats_new/modify_stairs_fill_edge_behaviour.rst
Fixing doc/devel/style_guide.rst
Fixing doc/users/next_whats_new/windows_arm64.rst
Fixing plot_types/basic/README.rst
Fixing doc/api/next_api_changes/deprecations/22268-OG.rst
Fixing doc/api/toolkits/mplot3d/faq.rst
Fixing .github/workflows/tests.yml
Fixing doc/_static/switcher.json
Fixing doc/_static/.gitignore
Fixing doc/api/next_api_changes/deprecations/22345-JK.rst
Fixing LICENSE/LICENSE_COURIERTEN
Fixing doc/users/next_whats_new/inset_axes_improvements.rst
Fixing plot_types/arrays/README.rst
Fixing LICENSE/LICENSE_BAKOMA
Fixing doc/api/lines_api.rst
Fixing LICENSE/LICENSE
Fixing doc/api/next_api_changes/development/22205-ES.rst
Fixing doc/api/next_api_changes/deprecations/22134-OG.rst
Fixing doc/api/_enums_api.rst
Fixing lib/matplotlib/backends/web_backend/css/page.css
Fixing plot_types/unstructured/README.rst
Fixing doc/api/next_api_changes/removals/21980-CC.rst
Fixing doc/users/next_whats_new/list_font_names.rst
Fixing doc/api/next_api_changes/behavior/22691-JMK.rst
Fixing LICENSE/LICENSE_CARLOGO
Fixing plot_types/stats/README.rst
Fixing doc/api/next_api_changes/removals/22516-OG.rst
Fixing doc/api/next_api_changes/behavior/22639-RA.rst
Fixing README.rst
Fixing doc/users/next_whats_new/layout_engine.rst
Fixing doc/api/next_api_changes/deprecations/22133-OG.rst
Fixing doc/missing-references.json
Fixing LICENSE/LICENSE_STIX

Mixed line ending........................................................Passed
Trim Trailing Whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing doc/users/next_whats_new/3d_plot_roll_angle.rst
Fixing doc/devel/contributing.rst
Fixing LICENSE/LICENSE_AMSFONTS
Fixing doc/users/next_whats_new/modify_stairs_fill_edge_behaviour.rst
Fixing .github/ISSUE_TEMPLATE/documentation.yml
Fixing doc/_templates/automodule.rst
Fixing doc/README.txt
Fixing doc/users/index.rst
Fixing .github/ISSUE_TEMPLATE/maintenance.yml
Fixing doc/users/explain/fonts.rst
Fixing doc/api/tri_api.rst
Fixing CODE_OF_CONDUCT.md
Fixing doc/api/next_api_changes/development/00001-ABC.rst
Fixing LICENSE/LICENSE_BAKOMA
Fixing doc/api/lines_api.rst
Fixing doc/users/github_stats.rst
Fixing doc/users/faq/index.rst
Fixing doc/api/next_api_changes/removals/00001-ABC.rst
Fixing LICENSE/LICENSE_COLORBREWER
Fixing doc/api/next_api_changes/behavior/00001-ABC.rst
Fixing src/_backend_agg.h
Fixing plot_types/unstructured/README.rst
Fixing doc/users/resources/index.rst
Fixing doc/api/next_api_changes/removals/21980-CC.rst
Fixing .github/ISSUE_TEMPLATE/bug_report.yml
Fixing lib/matplotlib/backends/web_backend/css/fbm.css
Fixing examples/event_handling/README.txt
Fixing doc/users/next_whats_new/extending_MarkerStyle.rst
Fixing doc/api/patches_api.rst
Fixing LICENSE/LICENSE_STIX

flake8...................................................................Passed
codespell................................................................Passed

which maybe explains @tacaswell's comment here #21583 (comment) more.

As these failing hooks are across a large number of files, if it would make it easier in a follow up PR to this one (or a new PR in which this is done in advance of this getting merged) I could apply them iand then add the commit to a .git-blame-ignore-revs which would then ignore those commits in the blame view (c.f. scikit-build/scikit-build#665 (comment) for how scikit-build uses this to avoid having large applications of black show up in blame). Or if that's not acceptable the

could get removed (#21583 (comment)).

Just blocking until we sort out how top make sure the pre-commit doesn't get out of sync with CI. So far options seem to be run precommit-ci or make reviewdog run a parallel set of checks.

The point is that we shouldn't merge PRs that fail the precommit-ci or we will have trivial follow-up PRs to correct things for pre-commit folks.

👍 Makes sense. Comments welcome on PR #22809 which should address all of this.

OK, I have turned pre-commit on for the main repo (it was already on for pytest-mpl and ipympl). I am not super stoked that it has the ability to push to the main repo, but it is not the only application with similar access.

OK, I have turned pre-commit on for the main repo (it was already on for pytest-mpl and ipympl).

And everything on main is passing:

and things are looking pretty good across the board

I am not super stoked that it has the ability to push to the main repo, but it is not the only application with similar access.

@tacaswell I think you already know this, so forgive me if I am over explaining, but this is because it has the ability to push back fixes if desired. As discussed before, this is turned off in the pre-commit config:

and as shown in matthewfeickert#9 it will only attempt to push back changes to a PR if it is explicitly asked for them.

You mentioned that it isn't the only application that has permissions to do this, but if the dev team isn't comfortable with this then you can simply disable it the same way you turned it on and a different approach can be used.

As I imagine there might be some future discussions on this I can open up a GitHub Issue for what is the desired CI system moving forward.

Overly permissive OAuth tokens on my mind due to the recent travis/heroku issue.

We do not have any private repos (which is what those attackers were going for), but you could in principle do something "interesting" if you could get your hands on a write token to the Matplotlib repo (and by "interesting" I mean "push malicious code"). However, unless it has admin write privileges all of the branches that are "special" (main, vX.Y.x, and *-doc) require a PR to push to.

However, unless it has admin write privileges all of the branches that are "special" (main, vX.Y.x, and *-doc) require a PR to push to.

Indeed, having admin privileges would be a security disaster.

I would agree that the GitHub permissions listing is not super explicit about which permissions level an associated app has

but I can tell you that pre-commit.ci does not have admin privileges on any of the orgs that I'm an owner of (e.g. Scikit-HEP). @henryiii and @jpivarski can check me on that/give a better explanation if I'm missing anything relevant in the details.

Hm. PR #22964 caught additional spelling errors. A large chunk of them I'm not surprised that codespell didn't catch as doc/api/prev_api_changes should be excluded by

but I'm not really sure why ploted -> plotted wasn't caught in

codespell does describe itself as trying to catch common misspellings

Fix common misspellings in text files. It's designed primarily for checking misspelled words in source code, but it can be used with other files as well. It does not check for word membership in a complete dictionary, but instead looks for a set of common misspellings. Therefore it should catch errors like "adn", but it will not catch "adnasdfasdf". This also means it shouldn't generate false-positives when you use a niche term it doesn't know about.

so I would have thought it would get things like this.

edit:

Guess not as

$ codespell --version
2.1.0
$ echo "ploted" | codespell -  # No error, so passes

I still think this is a net-positive to add. It will catch many common mistakes early on without a dev needing to comment on nitpick wording/styling updates.

It will catch many common mistakes early on without a dev needing to comment on nitpick wording/styling updates.

Agreed @greglucas. The codespell team merged my codespell-project/codespell#2341 PR very quickly, which makes me think that I could probably open up a PR to them that get most of the typos identified in PR #22964 so that in subsequent releases of codespell the pre-commit hook would also catch these.

Also the next release of codespell fixes the Big Sur issue (c.f. codespell-project/codespell#2299) so that could get removed from ci/codespell-ignore-words.txt in the future.

@jklymak gentle ping on this if you have time this week.

Sorry, been teaching an intensive course. Didn't mean to keep th block on if others are ok with this. Thanks for your patience.

@jklymak Nothing to be sorry about! Thanks very much for your continued and thoughtful feedback during the life cycle of this PR. 👍

Whoops, looks like I didn't rebase this against main this morning and so PR #22298 got in before this and introduced some typos.

$ pre-commit run codespell --all-files
codespell................................................................Failed
- hook id: codespell
- exit code: 65

lib/matplotlib/cm.py:157: exisiting ==> existing
lib/matplotlib/cm.py:238: compatbility ==> compatibility

I'll open up a fix for that.