search

tamasfe / taplo

A TOML toolkit written in Rust
https://taplo.tamasfe.dev
MIT License
1.32k stars 110 forks source link

Feature request: pre-commit hook for formatter #535

Open maresb opened 7 months ago

maresb commented 7 months ago

It would be great if there were a pre-commit hook for the TOML formatter. My current go-to formatter is pretty-format-toml, but this suffers from a fairly serious issue that comments are sometimes deleted. If this were implemented, I think it could easily become the best formatter with a pre-commit hook.

ia0 commented 7 months ago

Thanks for the suggestion! I guess this needs both the creation of some files in this repo and a PR for the pre-commit repository. This project is currently passively maintained, so only bugs are fixed. New functionalities need a PR contribution. Feel free to open one if you get the chance.

nikaro commented 7 months ago

@maresb you can use mine if want: https://github.com/nikaro/taplo-pre-commit

Beware that the Docker hooks don't work on arm64 machines, until the next release of Taplo, cf. this issue. The native Rust ones are fine, but the build will take a lot of time the first time you run it (after that it should be in your cache so it will be ok).

Feel free to reuse it to open a PR here if you want.

maresb commented 6 months ago

Thanks @nikaro, I just now got around to trying this out. My experience is that it was going very slowly during the initial install. My suspicion is that pre-commit was downloading a rust toolchain in the background, and that's why it's so slow.

I was wondering what's the purpose of your Docker hooks, but if I understood the above correctly then it saves on all the compile time. Running Docker from pre-commit seems pretty crazy to me. I wish it could just download binaries and use those. Unfortunately I'm not familiar with rust development, so I have no sense of the challenges of doing this.

Speaking of which, there are supposed to be binary releases but it seems like the links are broken and no binary artifacts are being generated by the GitHub workflows. Should I open a separate issue for that?

nikaro commented 6 months ago

Thanks @nikaro, I just now got around to trying this out. My experience is that it was going very slowly during the initial install. My suspicion is that pre-commit was downloading a rust toolchain in the background, and that's why it's so slow.

You are right that's what's happening: downloading rust toolchain and compiling. And that's why the -docker hook is here, to serve as a workaround to avoid having to compile (but it indeed requires Docker...).

I wish it could just download binaries and use those. Unfortunately I'm not familiar with rust development, so I have no sense of the challenges of doing this.

There is PR for that in pre-commit bit it seems a bit stale lately... :-/

Speaking of which, there are supposed to be binary releases but it seems like the links are broken and no binary artifacts are being generated by the GitHub workflows. Should I open a separate issue for that?

Yep, looks like the release workflow is broken: https://github.com/tamasfe/taplo/actions/runs/7742605055/workflow#L268 And it seems to be my fault 😬 the env.RELEASE_VERSION variable is not available in the matrix.

redeboer commented 6 months ago

A solution is being prepared in https://github.com/tamasfe/taplo/pull/549. That's just the Python package, providing a pre-commit hook is the next step and should be quite easy. A test version of such a hook is available in https://github.com/ComPWA/mirrors-taplo/issues/13#issuecomment-1930749688

mdekstrand commented 2 months ago

As a stopgap I quickly threw together a hook that uses the binaries published to NPM: https://github.com/mdekstrand/taplo-pre-commit