Skip to content

Preserve case-insensitive sorts in uv add #7864

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.

Sign up for GitHub

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

Merged
charliermarsh merged 4 commits into from
Oct 4, 2024
Merged

Preserve case-insensitive sorts in uv add #7864

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b6d079f
Preserve case-insensitive sorts in `uv add`
blueraft Oct 2, 2024
7b3de53
Respect case sensitive sorting in `uv add`
blueraft Oct 3, 2024
a758e84
Merge branch 'main' into add-sort
charliermarsh Oct 4, 2024
3ed9652
Use enum
charliermarsh Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions crates/uv-workspace/src/pyproject_mut.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,16 +539,31 @@ pub fn add_dependency(
// so that user's custom dependency ordering is preserved.
// Additionally, if the table is invalid (i.e. contains non-string values)
// we still treat it as unsorted for the sake of simplicity.
let sorted = deps.iter().all(toml_edit::Value::is_str)
// We account for both case-sensitive and case-insensitive sorting.
let all_strings = deps.iter().all(toml_edit::Value::is_str);

let case_insensitive_sorted = all_strings
&& deps.iter().tuple_windows().all(|(a, b)| {
a.as_str().map(str::to_lowercase) <= b.as_str().map(str::to_lowercase)
});

let case_sensitive_sorted = all_strings
&& deps
.iter()
.tuple_windows()
.all(|(a, b)| a.as_str() <= b.as_str());

let req_string = req.to_string();
let index = if sorted {
let index = if case_sensitive_sorted || case_insensitive_sorted {
deps.iter()
.position(|d: &Value| d.as_str() > Some(req_string.as_str()))
.position(|d: &Value| {
if case_insensitive_sorted {
d.as_str().map(str::to_lowercase)
> Some(req_string.as_str().to_lowercase())
} else {
d.as_str() > Some(req_string.as_str())
}
})
.unwrap_or(deps.len())
} else {
deps.len()
Expand Down
69 changes: 69 additions & 0 deletions crates/uv/tests/edit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4680,6 +4680,74 @@ fn sorted_dependencies() -> Result<()> {

let pyproject_toml = context.read("pyproject.toml");

insta::with_settings!({
filters => context.filters(),
}, {
assert_snapshot!(
pyproject_toml, @r###"
[project]
name = "project"
version = "0.1.0"
description = "Add your description here"
requires-python = ">=3.12"
dependencies = [
"anyio>=4.3.0",
"CacheControl[filecache]>=0.14,<0.15",
Copy link
Copy Markdown
Member

@charliermarsh charliermarsh Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might want to preserve either case-sensitive or case-insensitive sorts, rather than always using a case-insensitive sort. What do you think?

Copy link
Copy Markdown
Contributor Author

@blueraft blueraft Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, I'd probably prefer case insensitive sort as that's probably the most common case. In this example, I'd expect anyio to come before CacheControl. But happy to do it either way.

Copy link
Copy Markdown
Member

@charliermarsh charliermarsh Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to default to case insensitive, but if the user has intentionally sorted as case sensitive, I think it makes sense to preserve it?

Copy link
Copy Markdown
Contributor Author

@blueraft blueraft Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I've also added a test for the case-sensitive case.

"iniconfig",
"typing-extensions>=4.10.0",
]
"###
);
});
Ok(())
}

/// Ensure that the added dependencies are case sensitive sorted if the dependency list was already
/// case sensitive sorted prior to the operation.
#[test]
fn case_sensitive_sorted_dependencies() -> Result<()> {
let context = TestContext::new("3.12");

let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(indoc! {r#"
[project]
name = "project"
version = "0.1.0"
description = "Add your description here"
requires-python = ">=3.12"
dependencies = [
"CacheControl[filecache]>=0.14,<0.15",
"PyYAML",
"iniconfig",
]
"#})?;

uv_snapshot!(context.filters(), context.add().args(["typing-extensions", "anyio"]), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Resolved 14 packages in [TIME]
Prepared 13 packages in [TIME]
Installed 13 packages in [TIME]
+ anyio==4.3.0
+ cachecontrol==0.14.0
+ certifi==2024.2.2
+ charset-normalizer==3.3.2
+ filelock==3.13.1
+ idna==3.6
+ iniconfig==2.0.0
+ msgpack==1.0.8
+ pyyaml==6.0.1
+ requests==2.31.0
+ sniffio==1.3.1
+ typing-extensions==4.10.0
+ urllib3==2.2.1
"###);

let pyproject_toml = context.read("pyproject.toml");

insta::with_settings!({
filters => context.filters(),
}, {
Expand All @@ -4692,6 +4760,7 @@ fn sorted_dependencies() -> Result<()> {
requires-python = ">=3.12"
dependencies = [
"CacheControl[filecache]>=0.14,<0.15",
"PyYAML",
"anyio>=4.3.0",
"iniconfig",
"typing-extensions>=4.10.0",
Expand Down