Add --pypi option to pixi remove
#602
Conversation
There was a problem hiding this comment.
Hey @marcelotrevisani
Thanks for your work!
There are some things I would change. Let me know if my reviews need more explaining!
|
Thanks for reviewing this, I will do the modifications later today :) |
src/cli/remove.rs
Outdated
| .remove_target_dependency(dep, &spec_type, p) | ||
| if args.pypi { | ||
| let pkg_name = rip::types::PackageName::from_str(dep.as_source()) | ||
| .expect("Expected dependency name."); |
There was a problem hiding this comment.
Just like with the add functionality I think this cli subcommand should accept strings for deps. Parsing into a package name should yield an error if its not a valid package name. That way the user gets the best possible error message.
There was a problem hiding this comment.
I refactored that, please take a look again :)
marcelotrevisani
force-pushed
the
fix/591-pixi-remove-pypi-package
branch
from
f5f6caf
to
a8a1593
Compare
|
@marcelotrevisani As mentioned in discord, we would like to refactor the flow of the We would like it to first do all the input validation and then iterate over those results to remove them from the project. This would then also reintroduce the early out but with an actual error. Do you want to do this refactor or can I push to your branch? |
|
I can try to do it later on |
…section in pixi.toml
marcelotrevisani
force-pushed
the
fix/591-pixi-remove-pypi-package
branch
from
fbd7697
to
19fd138
Compare
src/cli/remove.rs
Outdated
| } else { | ||
| project.manifest.remove_pypi_dependency(dep)? | ||
| }; | ||
| print_ok_dep_removal( |
There was a problem hiding this comment.
Printing in this function will possibly give incorrect information to the user as you might error in a next iteration.
e.g.
pixi rm --pypi requests bla
# Results in:
Removed requests "*" from [pypi-dependencies]
× Couldn't find bla in pypi-dependencies
But as the save only happens later it will not actually be removed from the config.
There was a problem hiding this comment.
indeed, I will change that later today, thanks
Remove dependencies on pypi-dependencies section in pixi.toml
It fixes #591