-
Notifications
You must be signed in to change notification settings - Fork 61
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
pre-commit hook broken with CI #800
Comments
Alternatively someone could submit Julia as a supported language to https://github.com/pre-commit/pre-commit/blob/main/CONTRIBUTING.md#adding-support-for-a-new-hook-language. Then you could basically specify |
@MilesCranmer I looked into adding Julia as a language. Don't precisely recall why I didn't submit a PR for it, but I wager some of it had to do with the learning curve to getting it into I'm definitely happy to look back into this, but I won't be able to do so until around Feb or so. |
Thanks, I see. Maybe we can just implement a workaround in the command itself? Since there's only a single pre-commit hook available for Julia right now it might not (yet) be worth it to add it as a full language. Especially considering how much work it looks like to do so: pre-commit/pre-commit#751 |
There is this repo which implements julia-format pre-commit hook but the issue is that it assumes that JuliaFormatter is install in the current julia package environment.
It seemed relevant to the discussion here so wanted to share it. |
@senhalil thanks for sharing! Functionally, this is exactly like what the JuliaFormatter |
Thanks for the pointer @jmuchovej I didn't know. I'm new to pre-commit and julia. Is it possible to achieve what I asked in the other issue here somehow? |
@senhalil so, what @MilesCranmer proposed was that this All three of these things can be done. Not sure if @domluna is opposed to this (at least as a stand-in while Julia gains second party support in @domluna "this" refers to the
@senhalil I'm happy to do this, but I wont' have time to do so until February. Generally, I'm hoping to add 2nd-class support for Julia into |
@jmuchovej thanks for the useful context.
Exactly. Since we have multiple julia/non-julia projects, the dependencies are not installed in the default environment and qiaojunfeng/pre-commit-julia-format#2 (comment) already added what I needed so you can disregard my comment but I re-read your previous comment and you already gave me the solution I needed apparently 🤦
I don't even need
|
I don't have much of an opinion on pre-hooks or actions generally for that matter so I'd be happy to defer my judgement here and go with whatever the consensus here is. If someone wants to make a PR I'd be happy to help. |
Hm, @qiaojunfeng I think this still won't run on precommit.ci though because Julia is not installed. Since the python package juliacall installs julia automatically, maybe it makes sense to write a hook to install that instead, whereby juliacall would then install JuliaFormatter? i.e., specify the language as python with juliacall as one of the dependencies. So you should be able to do something like language: python
additional_dependencies: juliacall
entry: |
python -c '
from juliacall import Pkg, Main as jl
Pkg.activate("formatter", shared=True)
if "JuliaFormatter" not in Pkg.project().dependencies:
Pkg.add("JuliaFormatter")
jl.seval("using JuliaFormatter")
jl.JuliaFormatter.format(".")
' and it will automatically install (or simplify verify installation) both Julia and JuliaFormatter. |
Currently if you use Julia formatter as one of the pre-commit hooks, and turn on pre-commit.ci (https://pre-commit.ci/), you will encounter the following error:
https://results.pre-commit.ci/run/github/329653923/1704567683.9WG8haNCTo2d9L6yG9U5Qw
I think what needs to happen is the file https://github.com/domluna/JuliaFormatter.jl/blob/master/.pre-commit-hooks.yaml needs to:
julia
is available; if not, install it (probably with juliaup)--project=@jl-formater
); if not, install itformat
Right now the
.pre-commit-hooks.yaml
file assumes both julia and JuliaFormatter are installed. I think this is a reasonable assumption locally but it breaks pre-commit.ci which I think (?) is a fairly common way to use pre-commit in repositories.cc @jmuchovej
The text was updated successfully, but these errors were encountered: