-
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: add --priority arg to project channel add
#2086
Conversation
- allows specifying priority - fixes `channel remove` when priority is present in channels
186dca0
to
d7ed6b4
Compare
project channel add
project channel add
always recreate the list rather than parse, edit, unparse
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.
Thanks you!
This looks good. I would like to move the serialization code to the PrioritizedChannel code.
Also could you add some tests to verify that the channel priority is correctly added. This can be done in the pixi_manifest/src/manifests/manifest.rs
. There should be some tests you could mimic.
let mut table = Table::new().into_inline_table(); | ||
table.insert("channel", channel.channel.to_string().into()); | ||
table.insert("priority", i64::from(priority).into()); | ||
push(table.into()); |
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.
Instead of you could implement the toml_edit::Value
for this PrioritizedChannel
. Which then might be reused in different places.
6b86e1c
to
d531eb3
Compare
Moved the serialization, did my best to add tests. There didn't appear to be any tests for |
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.
Awesome work! I've added some small nitpicks but nice work on the test! Looks good to me! Great work on your first Rust PR!
Test fail because of a non related issue! |
Thanks for merging and thanks for the help! |
project channel remove
when priority is present in channels, which errored before.Draft because this is my first ever Rust PR, and I assume there must be a better way to turn a toml Value into a PrioritizedChannel than what I've done here. I'd love a pointer for the right way to do Value->PrioritizedChannel.
closes #2035