-
Notifications
You must be signed in to change notification settings - Fork 178
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: global update should add new executables #2298
feat: global update should add new executables #2298
Conversation
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.
Only a review of the code, since I didn't test it locally yet. I added a few comments.
In the description of this PR, you outlined quite nicely how the new behavior works. Could you please also do that in the docstring of the command?
src/global/common.rs
Outdated
let exposed_binaries_names = exposed_mapping_binaries | ||
.iter() | ||
.map(|mapping| mapping.executable_name()) | ||
.collect_vec(); |
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.
Same here
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.
^ here I think it will not work because I need to check if a name is in this exposed list. Changed it to hashset though ( instead of vec )
I've now tested it locally and it works fine. The tests on Windows are failing though. |
Co-authored-by: Hofer-Julian <[email protected]>
Co-authored-by: Hofer-Julian <[email protected]>
Co-authored-by: Hofer-Julian <[email protected]>
Co-authored-by: Hofer-Julian <[email protected]>
…://github.com/nichmor/pixi into feat/global-update-should-add-new-executables
This PR aims to change how pixi global update picks up and exposes new binaries.