-
-
Notifications
You must be signed in to change notification settings - Fork 781
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
Support for conda as a language #1232
Conversation
This is lacking sufficient tests, I'm unsure what should be tested and am open for suggestions. |
30ed0d1
to
79779be
Compare
This has now all review comments incorporated and tests are passing except for Windows. There we need to download a lightweight conda (take an exe from https://repo.anaconda.com/pkgs/misc/conda-execs/) to get the tests passing. This needs an modification of the Azure template like https://github.com/asottile/azure-pipeline-templates/blob/f8721c757e218487ca0bb38af0201b2a04e9ed52/job--python-tox.yml#L63-L68 @asottile Should I modify the template to always install a conda.exe on Windows or make this conditional on some parameter? |
there's a for example, here's what it does to install ruby / swift on linux: pre-commit/azure-pipelines.yml Lines 41 to 46 in 023b337
|
5aba042
to
f7144ad
Compare
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.
seems good!
@asottile Adjusted all things and CI is still green. :) |
pre_commit/languages/conda.py
Outdated
|
||
@contextlib.contextmanager | ||
def in_env(prefix, language_version): | ||
helpers.assert_version_default('conda', language_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 should be part of install_environment
I believe
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.
Should these be removed here then?
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.
yes -- you can take precedent from the other languages that use this function
ebdac73
to
4682d03
Compare
@asottile Azure is failing with missing files in the template repo but these actually exist. Do you have an idea what is going wrong here? |
huh they must have changed the inclusion rules :/ |
I'm trying to fix that in #1234 -- we'll see! |
ok if you rebase on master it should fix the CI |
4682d03
to
ba021aa
Compare
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.
ba021aa
to
4ff23b4
Compare
Fixes #1204
Example usage
.pre-commit-hooks.yaml
:environment.yml
: