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

Feature request: pre-commit hook for formatter #535

Open
maresb opened this issue Jan 20, 2024 · 5 comments
Open

Feature request: pre-commit hook for formatter #535

maresb opened this issue Jan 20, 2024 · 5 comments
Labels

Comments

@maresb
Copy link

maresb commented Jan 20, 2024

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
Copy link
Collaborator

ia0 commented Jan 20, 2024

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.

@ia0 ia0 added the feature label Jan 20, 2024
@nikaro
Copy link
Contributor

nikaro commented Jan 22, 2024

@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
Copy link
Author

maresb commented Feb 4, 2024

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
Copy link
Contributor

nikaro commented Feb 5, 2024

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
Copy link

redeboer commented Feb 8, 2024

A solution is being prepared in #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 ComPWA/mirrors-taplo#13 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants