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

constructor: remove pycosat, add pyyaml requirement #104

Closed
wants to merge 1 commit into from

Conversation

mbargull
Copy link

Removed dependency pycosat since this is not a direct requirement of constructor but one of libconda.
On the other hand pyyaml is a requirement for constructor and has been missing.

@ilanschnell
Copy link
Contributor

pycosat is a only an optional dependency of libconda, and pyyaml is a hard dependency of libconda. Therefore pycosat has to stay a dependency, and it is not necessary to add the pyyaml dependency.

@ilanschnell ilanschnell closed this Jun 6, 2017
@mbargull
Copy link
Author

mbargull commented Jun 6, 2017

I disagree on both points:

  • pyyaml is a direct dependency for constructor and thus must be added. Say you'd (for some unknown reason) remove the .condarc parsing from libconda and in turn remove the then unnecessary pyyaml dependency. This could in effect break constructor.
  • You could argue that pycosat is only an optional dependency because it is only needed if libconda.logic or libconda.resolve (which imports libconda.logic and thus pycosat) are used. But according to libconda's readme, libconda.resolve.Resolve is part of its "main functionality":

The main functionality this package provides is to allow importing the
following:

from libconda.fetch import fetch_index, fetch_pkg
from libconda.resolve import Resolve

Hence it sounds surprising that the main functionality is actually declared as being something optional.

@mbargull
Copy link
Author

mbargull commented Jun 6, 2017

To be able to remove pycosat as a dependency, #103 would need to be adopted beforehand, of course. Sorry I hadn't referenced #103 as a requirement for this before.

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

Successfully merging this pull request may close these issues.

2 participants