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

alphasort env files #557

Merged
merged 1 commit into from
Dec 13, 2023
Merged

alphasort env files #557

merged 1 commit into from
Dec 13, 2023

Conversation

jamesmyatt
Copy link
Contributor

Fixes #554

@jamesmyatt jamesmyatt requested a review from a team as a code owner November 20, 2023 20:48
Copy link

netlify bot commented Nov 20, 2023

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit d3b3d93
🔍 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
Copy link
Contributor Author

jamesmyatt commented Nov 20, 2023

There's already a test that the explicit files are toposorted:

def test_explicit_toposorted() -> None:
. 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
Copy link
Contributor

maresb commented Nov 20, 2023

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
Copy link
Contributor Author

jamesmyatt commented Nov 20, 2023

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
Copy link
Contributor Author

Seems to work on my machine 👍

@jamesmyatt jamesmyatt marked this pull request as ready for review November 20, 2023 22:22
@maresb maresb mentioned this pull request Nov 24, 2023
@maresb
Copy link
Contributor

maresb commented Nov 24, 2023

@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
Copy link
Contributor Author

jamesmyatt commented Nov 24, 2023

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

@maresb
Copy link
Contributor

maresb commented Dec 10, 2023

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
Copy link
Contributor Author

I've rebased now. Thanks

@maresb maresb merged commit b9b1ad1 into conda:main Dec 13, 2023
10 checks passed
@jamesmyatt jamesmyatt deleted the alphasort_env branch December 13, 2023 18:32
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.

Lock file for kind=env should be alphabetized
2 participants