Skip to content

fix: pre-commit hook#1465

Merged
Nytelife26 merged 5 commits intoamperser:mainfrom
cbachhuber:fix/pre-commit-hook
Jan 10, 2026
Merged

fix: pre-commit hook#1465
Nytelife26 merged 5 commits intoamperser:mainfrom
cbachhuber:fix/pre-commit-hook

Conversation

@cbachhuber
Copy link
Copy Markdown
Contributor

@cbachhuber cbachhuber commented Jan 7, 2026

Relevant issues

With proselint integrated into my pre-commit config like

  - repo: https://github.com/amperser/proselint
    rev: v0.16.0
    hooks:
      - id: proselint

I get

$ pre-commit run --all-files proselint
A linter for prose.......................................................Failed
- hook id: proselint
- exit code: 2

usage: proselint [options] <command>
proselint: error: argument : invalid choice: '.pyproject.toml' (choose from 'version', 'check', 'dump-config')
usage: proselint [options] <command>
proselint: error: argument : invalid choice: 'src/README.md' (choose from 'version', 'check', 'dump-config')
usage: proselint [options] <command>
proselint: error: argument : invalid choice: 'CONTRIBUTING.md' (choose from 'version', 'check', 'dump-config')

Brief

Fix the offered pre-commit hook.

Changes

Since proselint expects a list of files after the check command, pass that command to it in the pre-commit-hooks.yaml config, such that users don't have to. This fixes my manual test, do we want a continuous test for this?

@cbachhuber
Copy link
Copy Markdown
Contributor Author

Other repos such as https://github.com/crate-ci/typos/ document pre-commit integration, see https://github.com/crate-ci/typos/blob/master/docs/pre-commit.md. Should we do that in this repo as well?

@Nytelife26
Copy link
Copy Markdown
Contributor

Thank you for the contribution! I may not have overlooked this in the release if it were documented, so it would be nice to add that to the Plugins section of the README. Speaking of which, you are right, automated testing of the vendored plugins in question would be useful.

Would you like to open PRs for those too?

Nytelife26
Nytelife26 previously approved these changes Jan 7, 2026
@Nytelife26 Nytelife26 requested a review from drainpixie January 7, 2026 22:44
Copy link
Copy Markdown
Collaborator

@drainpixie drainpixie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the hook run on all text files? I fear this would return a lot of false-positives.

Naturally, right now proselint support for anything besides plain-text is "limited" (at least till docile progresses), but we could put some more standard file types till then, or document this caveat and let the user configure inputs themselves.

drainpixie
drainpixie previously approved these changes Jan 7, 2026
@Nytelife26
Copy link
Copy Markdown
Contributor

Nytelife26 commented Jan 8, 2026

Should the hook run on all text files? I fear this would return a lot of false-positives.

Thinking out loud here: if we had the machinery for rulesets (as in #1406), it could reduce friction for users to have different rulesets for non-plaintext files. I would be wary about this inconsistency constituting unexpected behaviour, so it might be better to turn off checks that cause issues for various file types by default.

we could put some more standard file types till then, or document this caveat and let the user configure inputs themselves.

We should narrow types to the permitted set defined in proselint.tools and files with no extension at least. I would also agree that proselint's limited support for non-plaintext files should be documented.

@cbachhuber cbachhuber dismissed stale reviews from drainpixie and Nytelife26 via 4a47cff January 8, 2026 20:01
Nytelife26
Nytelife26 previously approved these changes Jan 8, 2026
@Nytelife26 Nytelife26 requested a review from drainpixie January 8, 2026 20:04
@cbachhuber
Copy link
Copy Markdown
Contributor Author

Thanks, constrained file types in 4a47cff. I didn't include .rtf, as that's really unusual in git-based environments in my experience) and pre-commit's identify framework doesn't support this extension.

drainpixie
drainpixie previously approved these changes Jan 8, 2026
@cbachhuber cbachhuber dismissed stale reviews from drainpixie and Nytelife26 via 7cf714c January 8, 2026 20:08
@cbachhuber
Copy link
Copy Markdown
Contributor Author

@drainpixie @Nytelife26 these are the final changes in this PR, sorry for making your reviews stale: I updated the readme to demonstrate the pre-commit config. Is it ok to list this among the other installation options?

Signed-off-by: Chris Bachhuber <cbachhuber89@gmail.com>
@Nytelife26 Nytelife26 requested a review from drainpixie January 10, 2026 00:27
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.56%. Comparing base (505e07c) to head (4473f7d).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1465   +/-   ##
=======================================
  Coverage   92.56%   92.56%           
=======================================
  Files         100      100           
  Lines        1022     1022           
=======================================
  Hits          946      946           
  Misses         76       76           
Flag Coverage Δ
macos-latest 91.97% <ø> (ø)
py3.10 92.56% <ø> (ø)
py3.11 92.56% <ø> (ø)
py3.12 92.56% <ø> (ø)
py3.13 91.97% <ø> (ø)
py3.14 91.97% <ø> (ø)
ubuntu-latest 91.97% <ø> (ø)
windows-latest 92.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Nytelife26 Nytelife26 merged commit d19d1c8 into amperser:main Jan 10, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants