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

pre-commit hook broken with CI #800

Open
MilesCranmer opened this issue Jan 6, 2024 · 10 comments
Open

pre-commit hook broken with CI #800

MilesCranmer opened this issue Jan 6, 2024 · 10 comments

Comments

@MilesCranmer
Copy link
Contributor

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:

Screenshot 2024-01-06 at 19 13 06

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:

  1. Check if julia is available; if not, install it (probably with juliaup)
  2. Check if JuliaFormatter is installed in a particular environment (e.g., --project=@jl-formater); if not, install it
  3. Only then, import and run format

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

@MilesCranmer
Copy link
Contributor Author

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 JuliaFormatter as a dependency here and you'd be done...

@jmuchovej
Copy link
Contributor

@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 pre-commit. pre-commit/pre-commit#2689

I'm definitely happy to look back into this, but I won't be able to do so until around Feb or so.

@MilesCranmer
Copy link
Contributor Author

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

@senhalil
Copy link

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.

  - repo: https://github.com/qiaojunfeng/pre-commit-julia-format
    rev: v0.1.1
    hooks:
    - id: julia-format         # formatter for Julia code
      files: "project/.*jl"

It seemed relevant to the discussion here so wanted to share it.

@jmuchovej
Copy link
Contributor

@senhalil thanks for sharing! Functionally, this is exactly like what the JuliaFormatter pre-commit-hook.yaml does (as you can see here).

@senhalil
Copy link

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?

@jmuchovej
Copy link
Contributor

jmuchovej commented Jan 17, 2024

@senhalil so, what @MilesCranmer proposed was that this pre-commit hook install a missing Julia. It sounds like you're asking that JuliaFormatter be installed if it's a missing dependency (or by using JuliaFormatter in a different project environment)?

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 pre-commit (pre-commit/pre-commit#2689)).

@domluna "this" refers to the pre-commit-hook in JuliaFormatter also doing:

  1. Install missing Julia (using juliaup).
  2. Install missing JuliaFormatter (presumably in the @vX.Y project if unspecified).
  3. Support custom Julia project.

I'm definitely happy to look back into this, but I won't be able to do so until around Feb or so.

@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 pre-commit, but I don't have an ETA on that. 😅 (This would move, at least, (1) out of JuliaFormatter.)

@senhalil
Copy link

@jmuchovej thanks for the useful context.

using JuliaFormatter in a different project environment)?

Exactly. Since we have multiple julia/non-julia projects, the dependencies are not installed in the default environment and JuliaFormatter is installed inside a specific project.

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 🤦

this is exactly like what the JuliaFormatter pre-commit-hook.yaml does (as you can see here).

I don't even need pre-commit-julia-format I can modify the entry field.

- id: julia-formatter
  name: "Julia Formatter"
  entry: "julia --project=/path/to/project.toml -e 'import JuliaFormatter: format; format(\".\")'"
  ...

@domluna
Copy link
Owner

domluna commented Jan 19, 2024

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.

@MilesCranmer
Copy link
Contributor Author

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.

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

No branches or pull requests

4 participants