Initial implementation of uv add and uv remove#4193
Initial implementation of uv add and uv remove#4193ibraheemdev merged 22 commits intoastral-sh:mainfrom
uv add and uv remove#4193Conversation
| let upgrade = Upgrade::default(); | ||
| let extras = ExtrasSpecification::default(); | ||
| let exclude_newer = None; | ||
| let dev = false; // We only add regular dependencies currently. |
There was a problem hiding this comment.
I'm not sure if this is correct? Should dev dependencies ever be synced even if they weren't modified?
There was a problem hiding this comment.
It's possible that the dev dependencies could change by adding a non-dev dependency, because they're all locked together. (E.g., maybe the dep you add has a transitive dep on one of the dev deps, so now it's more constrained.) So, I guess we do have the include them? But it's not really right to sync them unconditionally. The same problem exists with extras. We shouldn't be syncing them unconditionally. We need to know which extras the user wants activated.
I'm wondering if add should imply lock, but not sync? Since locking is much simpler: we lock everything.
(I think there is a general problem here that exists as long as add implies lock or sync. (Rye has this problem too). Any arguments that are typically passed to lock or sync to guide resolution or installation now need to be passed to add too... So, e.g., we might need exclude-newer too.)
There was a problem hiding this comment.
Made add and remove do an unconditional full sync for now.
| let mut pyproject = PyProjectTomlMut::from_toml(project.current_project().pyproject_toml())?; | ||
| for req in requirements { | ||
| let req = Requirement::from(LenientRequirement::from_str(&req)?); | ||
| if pyproject.remove_dependency(&req)?.is_none() { |
There was a problem hiding this comment.
What happens if you uv remove anyio, without including the specifier?
There was a problem hiding this comment.
remove_dependency removes by package name, so that works.
|
|
||
| let mut pyproject = PyProjectTomlMut::from_toml(project.current_project().pyproject_toml())?; | ||
| for req in requirements { | ||
| let req = Requirement::from(LenientRequirement::from_str(&req)?); |
There was a problem hiding this comment.
I think we should enumerate some of the TODOs that aren't tackled yet. For example:
- We should accept unnamed URLs, and resolve them to named requirements.
- We need to figure out how this interacts with
tool.uv.sources. When do we add data totool.uv.sourcesrather thanproject.dependencies? \cc @konstin, you two should discuss this. - Editable requirements?
There was a problem hiding this comment.
I think we should just use pep508_rs::Requirement::from_str instead of LenientRequirement... We are a little inconsistent about it, but in general, LenientRequirement is really intended for third-party metadata over which the user has no control. We shouldn't let them add invalid requirements here.
There was a problem hiding this comment.
This also applies to the modification code in crates/uv-distribution/src/pyproject_mut.rs. What do you think?
There was a problem hiding this comment.
That makes sense to me, I removed the use of LenientRequirement.
crates/uv/src/cli.rs
Outdated
| pub(crate) struct RemoveArgs { | ||
| /// The packages to remove as PEP 508 requirement strings. e.g. 'flask==2.2.3' | ||
| #[arg(required = true)] | ||
| pub(crate) requirements: Vec<String>, |
There was a problem hiding this comment.
I wonder if this should be Requirement as long as we're parsing these directly. But, I think what you have here is probably better.
| let mut to_remove = None; | ||
| for (i, dep) in deps.iter().enumerate() { | ||
| if let Some(dep) = dep.as_str().and_then(|dep| parse_requirement(dep).ok()) { | ||
| if dep.name.as_ref().eq_ignore_ascii_case(req.name.as_ref()) { |
There was a problem hiding this comment.
What if a dep is included multiple times? E.g.:
dependencies = [
"flask > 2 ; python_version >= '3.6'",
"flask < 2 ; python_version < '3.6'",
]There was a problem hiding this comment.
Hmm it would only remove the first occurrence then. I think Rye has this bug as well?
There was a problem hiding this comment.
Updated to remove all occurrences. Also changed uv remove to take package names, not full requirements, because otherwise uv remove flask>2 becomes more complicated. I think package names make more sense anyways? Unless we want to support e.g. removing unnamed requirements by URL?
ibraheemdev
force-pushed
the
uv-add
branch
2 times, most recently
from
d934330 to
a955b35
Compare
|
|
||
| match to_replace { | ||
| Some(i) => { | ||
| deps.replace(i, req.to_string()); |
There was a problem hiding this comment.
What should happen if there are multiple occurrences? My guess is we replace them all.
There was a problem hiding this comment.
Replace them all with the single new requirement? i.e. replace the first occurrence and remove the rest?
There was a problem hiding this comment.
Implemented it that way, I think that makes the most sense, although it might be good to warn/log because this whole scenario is a bit questionable.
charliermarsh
left a comment
charliermarsh
left a comment
There was a problem hiding this comment.
A few more comments. I am happy to re-review but also trust you to address them and merge when you think they're closed out.
4 participants
Summary
Basic implementation of
uv addanduv removethat supports writing PEP508 requirements toproject.dependencies.First step for #3959 and #3960.