pre-commit: add initial config and workflow #440
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| - [ ] Have you updated `CHANGELOG.md` with relevant non-documentation file changes? | ||
| - [ ] Have you updated the documentation for this change? | ||
|
|
||
| ______________________________________________________________________ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| name: PreCommit | ||
|
|
||
| on: | ||
| pull_request: | ||
| push: | ||
| branches: [main] | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| PreCommit: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v5 | ||
| - uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: 3.x | ||
| - uses: pre-commit/action@v3.0.1 | ||
| - uses: pre-commit-ci/lite-action@v1.1.0 | ||
| if: always() | ||
| with: | ||
| msg: "pre-commit: apply automatic fixes" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # Recommended workflow: | ||
| # - `git add --verbose --patch` to stage changes | ||
| # - `git commit` to commit them | ||
| # - rinse and repeat, if pre-commit wants / applied changes | ||
| # | ||
| # To limit impact to just the `docs` subfolder, uncomment the below | ||
| # files: ^docs/ | ||
|
|
||
| repos: | ||
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v6.0.0 | ||
| hooks: | ||
| - id: trailing-whitespace | ||
| - id: end-of-file-fixer | ||
| - id: check-added-large-files | ||
|
|
||
| - repo: https://github.com/executablebooks/mdformat | ||
| rev: 0.7.22 | ||
| hooks: | ||
| - id: mdformat | ||
| additional_dependencies: | ||
| - mdformat-black | ||
| - mdformat-beautysh | ||
| - mdformat-gfm | ||
| - mdformat-myst | ||
| - setuptools # Necessary until beautysh fix their stuff, see: https://github.com/hukkin/mdformat/issues/442 | ||
|
|
||
| - repo: https://github.com/jackdewinter/pymarkdown | ||
| rev: v0.9.32 | ||
| hooks: | ||
| - id: pymarkdown | ||
|
Contributor
There was a problem hiding this comment. does it work fine with
Contributor
Author
There was a problem hiding this comment. They don't seem to clash, and I will use whatever you all think is right, just want to spread the use of |
||
| args: | ||
| - --config=docs/.sphinx/.pymarkdown.json | ||
| - fix | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll toss my two cents here, minding the TA and contributor experience. I'm hoping the questions I raise here are helpful to those voices. Maybe others can speak to context behind this work that I'm missing. Please let me know if so. :)
I think we ought to be very deliberate with the purpose and impact of pre-commit in our pack. Have we considered the separation between
make lintand pre-commit? Both add friction to the authoring workflow through different strategies. What does it mean if a linter is in one but not the other?mdformat, like prettier, is highly opinionated. TAs are also opinionated, and it takes us time to come to an agreement on things. For example, I have a principled stance for why hyphens are better list markers than asterisks, but to force all authors to change course on that overnight will take many by surprise, and become an immediate pain point.
make lint, by contrast, doesn't get in the way of my work until the CI stage. By introducing pre-commit, we'll be directly insinuating another set of checks, this time forceful ones, into our contributors' daily workflows.Regardless of what these hooks do, I think we're going to encounter challenges with visibility. Pre-commit has what I consider a major flaw: it doesn't actually communicate well that my commit -- my main unit of work -- has failed. It dumps many lines to the console, potentially a lot of lines if its checks find many problems, then exits. It's then not always clear what I should do next. Do I commit again, in the case of autoformatters (which I have to know ahead of time are in my repo)? Do I go and manually fix the files, hoping that I didn't miss anything? To me this feels like a regression from the concerted effort we put into switching to Vale as a writing aide, where we made its focus be on warning the author, giving them a chance to make a decision, not block them for trivial things. Maybe we could call
git statusafter it runs? But that feels like a band-aid.As a last but minor point, for repos where the docs are nested in a product, it won't always be obvious how to combine pre-commit configs. We'll need to provide guidance for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I'll try and cluster it, and my answers, in distinct themes, as maybe I should not have mixed things up as much:
mdformat
But whether pre-commit is in use or not, shouldn't affect this?
The advantage of this is that it can be transparent to them, and if there's agreement on the direction, is there value in not doing it "overnight"?
mdformatis just something that worked for me, so I brought it here under that umbrella. Maybe I should've just brought the whitespace bits to start with ;)pre-commit
As much as possible, I think we should move towards
pre-commit. It's a standard approach, less brittle than the Makefile we have. That said, the line may need to be drawn between what can be done on source and on the generated HTML.I personally consider that a bug, rather than feature :). You still get the choice, though - you don't have to
pre-commit installlocally, and leave CI to do its thing.More,
make lintdoesn't actually apply the auto fixes (whether local or at CI time), which e.g. might mean I get to come back to failed CI just to clear some whitespace issues…For me it's just one less thing to think about. The tooling takes care of formatting for me.
Does
make lintdo better? Maybe I don't get what you mean, but I would rather not commit things that need to be fixed.No, it actually applies all the fixes it can - you just need to review and commit them.
If a project already has pre-commit, they'll know what to do. If they don't, that's another win in my book - introducing them to it.
vale
I don't have the context here, but I also think we should separate tools like vale and just trivial formatting things.
If it only runs in CI, I won't see the warning anyway, unless it's blocking? And if it is blocking, then how is using pre-commit different?