Add jlfmt pre-commit hook#992
Conversation
|
@gdalle Since you did the last PR, could I trouble you to test this out and see if there are any issues? repos:
- repo: https://github.com/JuliaEditorSupport/JuliaFormatter.jl
rev: 3b196afa4a8ed92ba816edec6d122145e311ae35 # This PR.
hooks:
- id: "jlfmt" |
|
Okay, that should work! |
I'm not even sure what a "breaking change in the repo" would mean. As far as the Julia package is concerned, the tags on the repo don't really matter, it's the content of the |
| # https://github.com/JuliaLang/Pkg.jl/issues/4360 which is not | ||
| # backported to 1.12 yet |
There was a problem hiding this comment.
Should we ask for a backport on the issue?
There was a problem hiding this comment.
Yes, I had actually meant to investigate this in more depth because it seems like it has been backported: JuliaLang/Pkg.jl#4493 I'm not sure why the CI runner doesn't pick it up.
| name = "JuliaFormatter" | ||
| uuid = "98e50ef6-434e-11e9-1051-2b60c6c9e899" | ||
| version = "2.3.2" | ||
| version = "2.4.0" |
There was a problem hiding this comment.
See my comment about package versioning vs hook versioning. Here I think the bump might be justified because of a docs change, but in general it depends whether you consider the hook to be "part of" the Julia package, which is debatable. For example, it won't be installed when people do Pkg.add("JuliaFormatter").
There was a problem hiding this comment.
Right, thanks for the suggestions! There are indeed a few good options to decouple semver of the package from semver of the hook.
But also, I don't really envision that there would be much need to update the hooks much in the future, so my inclination right now is that it's not hugely important to sort it out, and we can continue to live in a regime where hook behaviour is in practice guaranteed by package semver but not officially documented. I think if there is a need to make a breaking change in the future then we can revisit this design?
There was a problem hiding this comment.
(I think if I were to change this design, I would probably use the tag prefixes, that seems like the most elegant way to solve it. I'd also be interested in finding out how that interacts with things like Dependabot, but that'd be a minor detail.)
| (If you have other pre-commit hooks, just add the `repo: ...` block to your pre-existing list of repos.) | ||
|
|
||
| **Note that `rev` controls the version of the _hook_ that is checked out; it does not control which version of JuliaFormatter is actually used to format your code.** | ||
| The version used to do the actual formatting is determined by the version of JuliaFormatter that is installed in your global Julia environment. |
There was a problem hiding this comment.
Not really in your global Julia environment, it's not in @v1.12 since it is an app.
There was a problem hiding this comment.
Good catch! I'll rephrase.
There was a problem hiding this comment.
Oh, hold on, this part is actually right because it's the old hook that uses julia -e 'using JuliaFormatter...'. That does indeed use the global env. We should really allow passing a project argument though.
There was a problem hiding this comment.
We should really allow passing a project argument though.
Actually this is already possible via pre-commit args, so I'll just document it and call it a day
|
In case you want to point people to the required steps for implementing pre-commit on their repos: JuliaDiff/SparseMatrixColorings.jl#320 |
| - repo: "https://github.com/JuliaEditorSupport/JuliaFormatter.jl" | ||
| rev: "v1.0.18" # or whatever the desired release is | ||
| - repo: https://github.com/JuliaEditorSupport/JuliaFormatter.jl | ||
| rev: baf84dcde3e7d39a3339fecb51a5d853f8aa35af |
There was a problem hiding this comment.
It seems a bit dangerous to specify the commit hash before the PR's merge commit?
There was a problem hiding this comment.
This is the current release (baf84dc), so it is fine, I have not reverse engineered SHA :-)
2 participants
Closes #633
Closes #990
This is essentially the same as #959, but with some changes:
It's a separate pre-commit hook, rather than replacing the old one, which would have been a breaking change.
It's a minor annoyance that the hooks are now tied to the repo and that means we can't have breaking changes in hooks without also releasing a breaking change in the repo. Runic sidesteps this by having a separate repo for the hooks. However, we can live with it.
Instead of calling
jlfmtdirectly, it calls a shell script which prints a nice error if jlfmt isn't found, and additionally allows a user to pass a custom path to a jlfmt executable if so desired. (It struck me that since jlfmt's CLI arguments were designed to be similar to Runic's, one could go rogue and instead give it the path to a Runic executable lol)I've basically rewritten the docs and added it to the sidebar (for some reason it wasn't being displayed there). I also took the opportunity to add docs on using PackageCompiler to speed up time-to-first-format.