Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create pre-commit hook for running tex-fmt #81

Merged
merged 4 commits into from
Jan 12, 2025

Conversation

bnctth
Copy link
Contributor

@bnctth bnctth commented Jan 10, 2025

What I did

I wanted to create a hook for pre-commit to format my LaTeX files on every commit, however this requires that the hook return a non-0 exit code when modifying the supplied files. So I created the --fail-on-change flag that simply formats the files and returns 1 when any of them changed -- like some kind of hybrid between --check and no --check.

While I was making my changes I also found a bug when running with multiple files which I fixed (more in the commit message of ea3460b).

How to try

You can either use the method written here or use it from my repo with the following .pre-commit-config.yaml:

repos:
-   repo: https://github.com/bnctth/tex-fmt
    rev: 60472ef651e6dd6a827759a88e10e49f7ce750f9
    hooks:
    -   id: tex-fmt

README.md Show resolved Hide resolved
@WGUNDERWOOD
Copy link
Owner

Thanks for this. I'm not sure I understand the motivation for the --fail-on-change feature: if the commit hook fails, is it not better for the formatting to be aborted so the user can perform a manual inspection if necessary? Thus the regular --check flag could be used.

However I'm keen to get pre-commit integration set up!

@bnctth
Copy link
Contributor Author

bnctth commented Jan 11, 2025

The hook must exit nonzero on failure or modify files.

"Failing" on modifying files is a requirement of pre-commit. The workflow this way is:

  • you add your changes
  • you commit
  • if formatting the files actually modifies them:
    • you get an error message and the commit is aborted
    • you can check what changed
    • add again
    • commit again, now without an error.

This way it formats the changed files automatically and gives the committer a chance to check if everything is okay. Personally I find this more comfortable than having to manually format my files (I'm lazy, I know), but whether this is the right default behavior is up to you. There are examples working similarly, like end-of-file-fixer and fix-byte-order-marker from the official pre-commit hooks repository, but also you can find examples where it only checks by default and you need to specify that you want your problems autofixed, like in pretty-format-json.

@WGUNDERWOOD
Copy link
Owner

WGUNDERWOOD commented Jan 12, 2025

Thanks I understand. Since there are quite a few files changed here, perhaps you can do the following:

  1. Create a new PR to implement the --fail-on-change flag on develop.
  2. Edit this PR to just contain the pre-commit content, and rebase it onto develop.

Feel free to suggest changes to the README.md in each PR, as you have done here, but please keep these as minimal as possible. Thanks again!

@bnctth
Copy link
Contributor Author

bnctth commented Jan 12, 2025

All right, done and done. I also changed the .pre-commit-hooks.yaml so the used flag can be overwritten so everyone can choose what method is preferable to them. See the readme about it.

Hit me up if you need anything else!

.pre-commit-hooks.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@WGUNDERWOOD WGUNDERWOOD merged commit f2d256d into WGUNDERWOOD:develop Jan 12, 2025
11 checks passed
@WGUNDERWOOD
Copy link
Owner

Thanks, one last question: will the check flag override the fail-on-change flag as described in the README?

@bnctth
Copy link
Contributor Author

bnctth commented Jan 12, 2025

Yes! Supplying the args field overrides the hook defaults. I tested it to be extra sure.

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.

2 participants