Add pygrep-hooks and mdformat to pre-commit hooks #9644
Conversation
| - [ ] Tests added | ||
| - [ ] User visible changes (including notable bug fixes) are documented in `whats-new.rst` | ||
| - [ ] New functions/methods are listed in `api.rst` | ||
| - \[ \] Closes #xxxx |
There was a problem hiding this comment.
Are we confident this still displays correctly?
There was a problem hiding this comment.
It seems to be fine, though I also find that surprising.
There was a problem hiding this comment.
Indeed!
Though it seems a bit confusing since people will be directly editing it...
| > [!NOTE] | ||
| This is the boolean "summarization matrix" referred to in the classic Iverson (1980, Section 4.3)\[^2\] and "nub sieve" in [various APLs](https://aplwiki.com/wiki/Nub_Sieve). | ||
|
|
||
| > \[!NOTE\] |
There was a problem hiding this comment.
Same here — are we sure these should be escaped? Looks like a directive (though not sure what...)
|
|
||
| ## Copyright | ||
|
|
||
| This document has been placed in the public domain. | ||
|
|
||
| ## References and footnotes | ||
|
|
||
| [^1]: Wickham, H. (2011). The split-apply-combine strategy for data analysis. https://vita.had.co.nz/papers/plyr.html | ||
| [^2]: Iverson, K.E. (1980). Notation as a tool of thought. Commun. ACM 23, 8 (Aug. 1980), 444–465. https://doi.org/10.1145/358896.358899 | ||
| \[^1\]: Wickham, H. (2011). The split-apply-combine strategy for data analysis. https://vita.had.co.nz/papers/plyr.html |
There was a problem hiding this comment.
and here; seems unlikely this is correct
There was a problem hiding this comment.
A bunch of changes in this file look quite good!
|
Overall looks great! We should check some of those, I added a couple of comments, though not a complete review. To the extent that some might interfere with The markdown & rst checkers look overall great! |
| - repo: https://github.com/executablebooks/mdformat | ||
| rev: 0.7.18 | ||
| hooks: | ||
| - id: mdformat |
There was a problem hiding this comment.
apparently, we can fix (some of) the surprising bracket escapes with
| - id: mdformat | |
| - id: mdformat | |
| additional_dependencies: [mdformat-gfm] |
There was a problem hiding this comment.
though I wonder if instead of using mdformat we should rely on prettier to format markdown, as that can also format json and yaml?
There was a problem hiding this comment.
FWIW in prql we quite successfully use prettier to format all non-core code, including yaml / json / md; would recommend: https://github.com/PRQL/prql/blob/86becc591eb1a6421695ccccdf67dd8ab5c775ec/.pre-commit-config.yaml#L18-L22
...and it has a nice VS Code extension (and I guess emacs for the pros among us...)
|
Why don't we instead try to rely on ruff and enable these rules there? |
Definitely preferable for the python ones. (though formatting non-python files would still be part of this sort of effort...) |
| git pull upstream main | ||
| ``` | ||
|
|
||
| 1. Add a list of contributors. |
There was a problem hiding this comment.
Forgotten the enumeration again.
There was a problem hiding this comment.
This is rendering fine. It's a trick to have the renderer do the numbering without having to do it manually
There was a problem hiding this comment.
the downside of that trick is that reading the source becomes a bit harder to read (which item from the top was that again?), so I'd prefer a linter / autoformatter to keep source and rendered version as close as possible.
Given that prettier (mirror) appears to get this right, maybe we should be using that instead? That would also auto-format yaml, css and json, and for toml we can try taplo (mirror).
| 2. The CI job will be started. Checks will appear in the usual dashboard panel above the comment box. | ||
| 3. If more commits are added, the label checks will be grouped with the last commit checks _before_ you added the label. | ||
| 4. Alternatively, you can always go to the `Actions` tab in the repo and [filter for `workflow:Benchmark`](https://github.com/scikit-image/scikit-image/actions?query=workflow%3ABenchmark). Your username will be assigned to the `actor` field, so you can also filter the results with that if you need it. | ||
| 1. The CI job will be started. Checks will appear in the usual dashboard panel above the comment box. |
Towards #8239. This mainly addresses problems in rst and markdown files.