search

conda / conda-lock

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

alphasort env files #557

Closed jamesmyatt closed 6 months ago

jamesmyatt commented 7 months ago

Fixes #554

netlify[bot] commented 7 months ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit d3b3d933bf6214e33ca9684c5a3a4de6d73ac20e
Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/657981229acc05000819c76f
Deploy Preview https://deploy-preview-557--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 configuration.

jamesmyatt commented 7 months ago

There's already a test that the explicit files are toposorted: https://github.com/conda/conda-lock/blob/5bce4228cbd0af96ae00ec8efe0b9f2eb694ab53/tests/test_conda_lock.py#L995. Does it need a test that the env files are alphasorted?

Also should this use the KIND_... constants instead of literals?

Also I think the Union[Literal[...], ...] type hints can be simplified to Literal[..., ...]. Is that right?

maresb commented 7 months ago

If we want that the output just happens to be alphabetically sorted, then I'd say we don't need a test. But if we declare that alpha-sorting is an actual feature, then we should add a test to ensure it doesn't regress.

Also should this use the KIND_... constants instead of literals?

These constants date to before my involvement with conda-lock. I'm not familiar with them. I'd only recommend using them if they're helpful.

Also I think the Union[Literal[...], ...] type hints can be simplified to Literal[..., ...]. Is that right?

That is correct. I am always grateful for such cleanups. We could also remove them if they're not serving a useful purpose.

jamesmyatt commented 7 months ago

I don't see any documentation or tests in the conda repo on the ordering of the exported env, so there's no guarantee that won't change either. So we can probably claim that it's a choice rather than a feature.

I'll leave the constants and the type hints alone. Both could do with bigger PRs. I think rather than 1 method at a time.

jamesmyatt commented 7 months ago

Seems to work on my machine 👍

maresb commented 7 months ago

@jamesmyatt, in order to address a likely bug I made #560 which will cause a merge conflict. Resolving it will be easy: call lockfile.filter_virtual_packages_inplace() after sorting.

Before the next release I need to address #559. If you feel especially motivated, I'd be grateful for support with writing a test there, but otherwise I'll get to it eventually.

jamesmyatt commented 7 months ago

Thanks @maresb. I'll rebase this after you merge #560

maresb commented 6 months ago

Hi @jamesmyatt, sorry about the delay on #560. That's now been merged, so this will be ready to merge after you get a chance to rebase. Thanks for your patience!

jamesmyatt commented 6 months ago

I've rebased now. Thanks