Skip to content
This repository has been archived by the owner on Apr 1, 2022. It is now read-only.

Conda support #226

Merged
merged 10 commits into from
Jun 3, 2021
Merged

Conda support #226

merged 10 commits into from
Jun 3, 2021

Conversation

anuccio1
Copy link
Contributor

This PR adds support for conda. We will support it with 2 strategies:

  • conda list: this will output the dependencies to stdout
  • environment.yml: if conda list fails, then we fall back to parsing a yml file

Copy link
Contributor

@skilly-lily skilly-lily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a lot of comments, mostly small stuff like simplifications.

The two critical changes are combineSuccessful and not using head.

I also have a sneaking suspicion that you have not set up the linter and formatter. We use hlint for linting and ormolu for formatting. haskell-language-server integrates with both if they're on the PATH, and configured correctly (vscode is the reference for configuration, I think).

src/Strategy/Conda/EnvironmentYml.hs Outdated Show resolved Hide resolved
src/Strategy/Conda/EnvironmentYml.hs Outdated Show resolved Hide resolved
src/Strategy/Conda/EnvironmentYml.hs Outdated Show resolved Hide resolved
src/Strategy/Conda/EnvironmentYml.hs Show resolved Hide resolved
src/Strategy/Conda/EnvironmentYml.hs Show resolved Hide resolved
test/Conda/EnvironmentYmlSpec.hs Outdated Show resolved Hide resolved
src/Strategy/Conda.hs Outdated Show resolved Hide resolved
src/Strategy/Conda.hs Show resolved Hide resolved
src/Strategy/Conda.hs Outdated Show resolved Hide resolved
src/Strategy/Conda.hs Outdated Show resolved Hide resolved
@anuccio1 anuccio1 marked this pull request as ready for review May 11, 2021 20:48
@anuccio1 anuccio1 requested a review from cnr May 11, 2021 21:14
Copy link
Contributor

@skilly-lily skilly-lily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good to me.

I left a bunch of doc comments as nits, but the formatting and naming of analyze functions should probably be taken care of before merging.

src/Data/List/Extra.hs Show resolved Hide resolved
src/Strategy/Conda.hs Outdated Show resolved Hide resolved
src/Strategy/Conda.hs Outdated Show resolved Hide resolved
src/Strategy/Conda/CondaList.hs Outdated Show resolved Hide resolved
src/Strategy/Conda/EnvironmentYml.hs Outdated Show resolved Hide resolved
src/Strategy/Conda/CondaList.hs Show resolved Hide resolved
src/Strategy/Conda/EnvironmentYml.hs Show resolved Hide resolved
test/Conda/CondaListSpec.hs Show resolved Hide resolved
test/Conda/EnvironmentYmlSpec.hs Show resolved Hide resolved
src/Strategy/Conda/EnvironmentYml.hs Outdated Show resolved Hide resolved
@skilly-lily
Copy link
Contributor

Also, please do not merge this until we have merged and deployed the conda fetcher. Doing so may cause user confusion and/or block spectrometer releases.

@cnr cnr removed their request for review May 21, 2021 16:01
@anuccio1 anuccio1 merged commit 3fba84b into master Jun 3, 2021
@anuccio1 anuccio1 deleted the conda-support branch June 3, 2021 21:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants