conda / conda-lock

Lightweight lockfile for conda environments
https://conda.github.io/conda-lock/
Other
467 stars 102 forks source link

Refactor LockSpecification as a Dictionary from Platforms to List of Deps #383

Closed srilman closed 1 year ago

srilman commented 1 year ago

Description

As discussed in https://github.com/conda/conda-lock/pull/374, https://github.com/conda/conda-lock/issues/278, and previously implemented in https://github.com/conda/conda-lock/pull/316, this PR refactors the LockSpecification class to store all dependencies as a dictionary from platforms to a list of dependencies required for the platform.

After this PR, it is no longer necessary to have the Selector class, so it is also removed in this PR.

This PR still maintains source parsing code to return LockSpecification classes. This is not an ideal solution, since it requires parsing the source file multiple times. In the future, I will implement a PR to have a custom SourceFile class, like implemented in https://github.com/conda/conda-lock/pull/316.

netlify[bot] commented 1 year ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit 3ce07dc4c8aae03faa724bdf79d3d53333cd591b
Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/64051de18a77c40008699c44
Deploy Preview https://deploy-preview-383--conda-lock.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

maresb commented 1 year ago

This is magnificent!!! I love that we were able to delete so much code. It's nice to see the CI green. And now the code logic agrees with my mental model. All this makes very happy. :smile:

During my review I cleaned up some of the preexisting code a bit further: https://github.com/srilman/conda-lock/pull/1. If you agree, let's wrap that into this PR by merging on your fork.

I've reviewed this thoroughly, and from my side this is good to go.

Since this is such a substantial structural change, I think we should get approval from @mariusvniekerk.

srilman commented 1 year ago

@maresb I'm happy with the additional code cleanup in my local branch. I'm going to merge your branch into this one.

maresb commented 1 year ago

For orientation as to how this fits into the big picture:

What conda-lock does in a nutshell:

This PR reworks the internally-used LockSpecification data structure so that its implementation is more in line with how it's actually used.

srilman commented 1 year ago

To clarify further, this will not change how we parse input files or the resulting output lockfile in any way. This will only change an internal data structure so that it is easier to implement future changes. For example, I was working on a future PR to support encoding multiple categories in the lockfile, but kept running into bugs because of how we grouped all platforms together in the LockSpecification.