-
-
Notifications
You must be signed in to change notification settings - Fork 910
improve julia language support #3494
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
Conversation
for more information, see https://pre-commit.ci
asottile
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than doing 3 or 4 things in a pr please split this up to do one thing at a time with clear tests demonstrating the reason for changes
pre_commit/languages/julia.py
Outdated
| ) | ||
| ] | ||
| if version not in ('system', 'default'): | ||
| patches.append(('JULIAUP_CHANNEL', version)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't quite right. to properly support language_version pre-commit must install the language itself into the hook environment (and not outside of it such as in the juliaup managed directories)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, pre-commit wants an entire copy of julia installed into the hook environment directory? that's very unusual for julia, which uses content-addressed read-only files and packages to allow reuse across environments. It would also make pre-commit install much slower for julia hooks than it currently is.
|
|
||
| def _make_src_hook(tmp_path, pkg_code, script_code): | ||
| # here we have a package with a src dir and a script dir | ||
| src_dir = tmp_path.joinpath('src') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like from this test this should get handled by the install process. the precedence of copying little bits out of this seems not great and we should find a way to do this step by step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in the tests, so we're setting up an example with test files which do get copied as part of the install process
This PR fixes three issues I ran into when trying to use pre-commit's native Julia language support for ExplicitImports.jl (JuliaTesting/ExplicitImports.jl#100) in Pkg (JuliaLang/Pkg.jl#4326):
--startup-file=noto both invocations of Julia (install & run). The startup file is a Julia script that users can configure to run commands before the start of their Julia session. It is typically used to load developer packages like Revise and not used "in-production". Here when we run our pre-commit hooks we do not want to run the user's startup file which could have dependencies the current environment does not have.srcfiles in addition to Project/Manifest. This allows pre-commit hooks to live inside a package (like https://github.com/JuliaTesting/ExplicitImports.jl/blob/main/.pre-commit-hooks.yaml) instead of being in a separate repo (like https://github.com/fredrikekre/runic-pre-commit).JULIAUP_CHANNELenvironmental variable. This support is limited in a couple ways:JULIAUP_CHANNELdoes not do anything and setting thelanguage_versionwill not have any effect. That however is the status quo (currently setting thelanguage_versiondoes not have any effect) and many users use juliaup, so I think we could just document thatlanguage_versionsupport requires yourjuliato be from juliaup.With these changes, JuliaLang/Pkg.jl#4329 can run successfully.