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 thatcodespell thinks is a mispelling of a common word. For example, if you had a variable namedalgo things would be fine

diff --git a/lib/matplotlib/artist.py b/lib/matplotlib/artist.pyindex 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-filescodespell................................................................Passed

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

diff --git a/lib/matplotlib/artist.py b/lib/matplotlib/artist.pyindex 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-filescodespell................................................................Failed- hook id: codespell- exit code: 65lib/matplotlib/artist.py:62: mapp ==> map

and then you would need to addmapp 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.pyindex 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-filescodespell................................................................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, andlib/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 fromcodespell-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 spellingcorrections, 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 thatcodespell thinksmight 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 itcodespell fails with

$pre-commit run codespell --all-filescodespell................................................................Failed- hook id: codespell- exit code: 65lib/matplotlib/backends/backend_pdf.py:1651: Flate ==> Flat

You should also add (if not already automatically ignored by e.g.,.gitignore),extern,build, andlib/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-filescodespell................................................................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, andlib/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

Theextern directoryis in version control.

Theextern directoryis 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 ofextern/* isn't needed as it is already universally excluded bypre-commit

Another note: Givencodespell-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 enablingpre-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 ofcodespell? 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-filescodespell................................................................Passed

but I believe (I could be wrong, as I'm used to havingpre-commit.ci run withautofix_prs: true) thatpre-commit.ci runspre-commit run --all-files (as opposed to justpre-commit run which only hits the diff), which currently fails

$pre-commit run --all-filesCheck for added large files..............................................PassedCheck docstring is first.................................................Failed- hook id: check-docstring-first- exit code: 1lib/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 hookFixing plot_types/README.rstFixing doc/api/transformations.rstFixing lib/matplotlib/tests/READMEFixing doc/devel/testing.rstFixing doc/users/next_whats_new/modify_stairs_fill_edge_behaviour.rstFixing doc/devel/style_guide.rstFixing doc/users/next_whats_new/windows_arm64.rstFixing plot_types/basic/README.rstFixing doc/api/next_api_changes/deprecations/22268-OG.rstFixing doc/api/toolkits/mplot3d/faq.rstFixing .github/workflows/tests.ymlFixing doc/_static/switcher.jsonFixing doc/_static/.gitignoreFixing doc/api/next_api_changes/deprecations/22345-JK.rstFixing LICENSE/LICENSE_COURIERTENFixing doc/users/next_whats_new/inset_axes_improvements.rstFixing plot_types/arrays/README.rstFixing LICENSE/LICENSE_BAKOMAFixing doc/api/lines_api.rstFixing LICENSE/LICENSEFixing doc/api/next_api_changes/development/22205-ES.rstFixing doc/api/next_api_changes/deprecations/22134-OG.rstFixing doc/api/_enums_api.rstFixing lib/matplotlib/backends/web_backend/css/page.cssFixing plot_types/unstructured/README.rstFixing doc/api/next_api_changes/removals/21980-CC.rstFixing doc/users/next_whats_new/list_font_names.rstFixing doc/api/next_api_changes/behavior/22691-JMK.rstFixing LICENSE/LICENSE_CARLOGOFixing plot_types/stats/README.rstFixing doc/api/next_api_changes/removals/22516-OG.rstFixing doc/api/next_api_changes/behavior/22639-RA.rstFixing README.rstFixing doc/users/next_whats_new/layout_engine.rstFixing doc/api/next_api_changes/deprecations/22133-OG.rstFixing doc/missing-references.jsonFixing LICENSE/LICENSE_STIXMixed line ending........................................................PassedTrim Trailing Whitespace.................................................Failed- hook id: trailing-whitespace- exit code: 1- files were modified by this hookFixing doc/users/next_whats_new/3d_plot_roll_angle.rstFixing doc/devel/contributing.rstFixing LICENSE/LICENSE_AMSFONTSFixing doc/users/next_whats_new/modify_stairs_fill_edge_behaviour.rstFixing .github/ISSUE_TEMPLATE/documentation.ymlFixing doc/_templates/automodule.rstFixing doc/README.txtFixing doc/users/index.rstFixing .github/ISSUE_TEMPLATE/maintenance.ymlFixing doc/users/explain/fonts.rstFixing doc/api/tri_api.rstFixing CODE_OF_CONDUCT.mdFixing doc/api/next_api_changes/development/00001-ABC.rstFixing LICENSE/LICENSE_BAKOMAFixing doc/api/lines_api.rstFixing doc/users/github_stats.rstFixing doc/users/faq/index.rstFixing doc/api/next_api_changes/removals/00001-ABC.rstFixing LICENSE/LICENSE_COLORBREWERFixing doc/api/next_api_changes/behavior/00001-ABC.rstFixing src/_backend_agg.hFixing plot_types/unstructured/README.rstFixing doc/users/resources/index.rstFixing doc/api/next_api_changes/removals/21980-CC.rstFixing .github/ISSUE_TEMPLATE/bug_report.ymlFixing lib/matplotlib/backends/web_backend/css/fbm.cssFixing examples/event_handling/README.txtFixing doc/users/next_whats_new/extending_MarkerStyle.rstFixing doc/api/patches_api.rstFixing LICENSE/LICENSE_STIXflake8...................................................................Passedcodespell................................................................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 thenignore those commits in the blame view (c.f.scikit-build/scikit-build#665 (comment) for howscikit-build uses this toavoid having large applications ofblack 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 onmain 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 thepre-commit config:

and as shown inmatthewfeickert#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 hasadmin write privileges all of the branches that are "special" (main,vX.Y.x, and*-doc) require a PR to push to.

However, unless it hasadmin 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 thatcodespell didn't catch asdoc/api/prev_api_changes should be excluded by

but I'm not really sure whyploted ->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 --version2.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. Thecodespell team merged mycodespell-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 ofcodespell the pre-commit hook would also catch these.

Also the next release ofcodespell fixes the Big Sur issue (c.f.codespell-project/codespell#2299) so that could get removed fromci/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 againstmain this morning and so PR#22298 got in before this and introduced some typos.

$pre-commit run codespell --all-filescodespell................................................................Failed- hook id: codespell- exit code: 65lib/matplotlib/cm.py:157: exisiting ==> existinglib/matplotlib/cm.py:238: compatbility ==> compatibility

I'll open up a fix for that.