-
Notifications
You must be signed in to change notification settings - Fork 123
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
feat: initial minimal virtual package support #36
Conversation
ruben-arts
commented
May 30, 2023
- Add a minimal version of the virtual packages (excluding the Cuda one)
- Add a check to find if the current platform is at least matching the minimal packages for the platform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! I only have a few small notes.
virtual_packages | ||
} | ||
|
||
pub fn verify_current_platform_has_minimal_virtual_package_requirements( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function is named a bit weird. It implies it will check the current platform but you actually pass in the platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also its missings docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now checking the current platform itself and added some docs
src/virtual_packages.rs
Outdated
/// Define a reasonable modern set of virtual packages that should be safe enough to assume. | ||
/// On design this is in sync with the conda-lock set of default packages. | ||
/// https://github.com/conda/conda-lock/blob/3d36688278ebf4f65281de0846701d61d6017ed2/conda_lock/virtual_package.py#L175 | ||
pub fn get_minimal_virtual_packages(platform: Platform) -> Vec<VirtualPackage> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function comments should always be written in the imperative form. e.g. "Returns a reasonable modern set of ..." instead of "Return a reasonable set ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks better now