Skip to content

Commit

Permalink
Add various grammar changes to conflict error messages (#9369)
Browse files Browse the repository at this point in the history 
## Summary

If all items are the same kind, we can avoid repeating "extra" and
"group". If there are two, we now use "X and Y", etc.
  • Loading branch information
@charliermarsh
charliermarsh authored Nov 22, 2024
1 parent 619ec8d commit b2bad8d
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 64 deletions.
13 changes: 7 additions & 6 deletions crates/uv-python/src/discovery.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2422,31 +2422,31 @@ impl DiscoveryPreferences {
.map(ToString::to_string)
.collect::<Vec<_>>();
match self.environment_preference {
EnvironmentPreference::Any => conjunction(
EnvironmentPreference::Any => disjunction(
&["virtual environments"]
.into_iter()
.chain(python_sources.iter().map(String::as_str))
.collect::<Vec<_>>(),
),
EnvironmentPreference::ExplicitSystem => {
if request.is_explicit_system() {
conjunction(
disjunction(
&["virtual environments"]
.into_iter()
.chain(python_sources.iter().map(String::as_str))
.collect::<Vec<_>>(),
)
} else {
conjunction(&["virtual environments"])
disjunction(&["virtual environments"])
}
}
EnvironmentPreference::OnlySystem => conjunction(
EnvironmentPreference::OnlySystem => disjunction(
&python_sources
.iter()
.map(String::as_str)
.collect::<Vec<_>>(),
),
EnvironmentPreference::OnlyVirtual => conjunction(&["virtual environments"]),
EnvironmentPreference::OnlyVirtual => disjunction(&["virtual environments"]),
}
}
}
Expand All @@ -2471,8 +2471,9 @@ impl fmt::Display for PythonNotFound {
}

/// Join a series of items with `or` separators, making use of commas when necessary.
fn conjunction(items: &[&str]) -> String {
fn disjunction(items: &[&str]) -> String {
match items.len() {
0 => String::new(),
1 => items[0].to_string(),
2 => format!("{} or {}", items[0], items[1]),
_ => {
Expand Down
36 changes: 36 additions & 0 deletions crates/uv/src/commands/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,3 +257,39 @@ impl<'a> OutputWriter<'a> {
Ok(())
}
}

/// Given a list of names, return a conjunction of the names (e.g., "Alice, Bob, and Charlie").
pub(super) fn conjunction(names: Vec<String>) -> String {
let mut names = names.into_iter();
let first = names.next();
let last = names.next_back();
match (first, last) {
(Some(first), Some(last)) => {
let mut result = first;
let mut comma = false;
for name in names {
result.push_str(", ");
result.push_str(&name);
comma = true;
}
if comma {
result.push_str(", and ");
} else {
result.push_str(" and ");
}
result.push_str(&last);
result
}
(Some(first), None) => first,
_ => String::new(),
}
}

/// Capitalize the first letter of a string.
pub(super) fn capitalize(s: &str) -> String {
let mut chars = s.chars();
match chars.next() {
None => String::new(),
Some(c) => c.to_uppercase().collect::<String>() + chars.as_str(),
}
}
93 changes: 72 additions & 21 deletions crates/uv/src/commands/project/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use uv_workspace::{ProjectWorkspace, Workspace};
use crate::commands::pip::loggers::{InstallLogger, ResolveLogger};
use crate::commands::pip::operations::{Changelog, Modifications};
use crate::commands::reporters::{PythonDownloadReporter, ResolverReporter};
use crate::commands::{pip, SharedState};
use crate::commands::{capitalize, conjunction, pip, SharedState};
use crate::printer::Printer;
use crate::settings::{InstallerSettingsRef, ResolverInstallerSettings, ResolverSettingsRef};

Expand Down Expand Up @@ -221,26 +221,77 @@ pub(crate) struct ConflictError {

impl std::fmt::Display for ConflictError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(
f,
"{} are incompatible with the declared conflicts: {{{}}}",
self.conflicts
.iter()
.map(|conflict| match conflict {
ConflictPackage::Extra(ref extra) => format!("extra `{extra}`"),
ConflictPackage::Group(ref group) if self.dev.default(group) =>
format!("group `{group}` (enabled by default)"),
ConflictPackage::Group(ref group) => format!("group `{group}`"),
})
.join(", "),
self.set
.iter()
.map(|item| match item.conflict() {
ConflictPackage::Extra(ref extra) => format!("`{}[{}]`", item.package(), extra),
ConflictPackage::Group(ref group) => format!("`{}:{}`", item.package(), group),
})
.join(", ")
)
// Format the set itself.
let set = self
.set
.iter()
.map(|item| match item.conflict() {
ConflictPackage::Extra(ref extra) => format!("`{}[{}]`", item.package(), extra),
ConflictPackage::Group(ref group) => format!("`{}:{}`", item.package(), group),
})
.join(", ");

// If all the conflicts are of the same kind, show a more succinct error.
if self
.conflicts
.iter()
.all(|conflict| matches!(conflict, ConflictPackage::Extra(..)))
{
write!(
f,
"Extras {} are incompatible with the declared conflicts: {{{set}}}",
conjunction(
self.conflicts
.iter()
.map(|conflict| match conflict {
ConflictPackage::Extra(ref extra) => format!("`{extra}`"),
ConflictPackage::Group(..) => unreachable!(),
})
.collect()
)
)
} else if self
.conflicts
.iter()
.all(|conflict| matches!(conflict, ConflictPackage::Group(..)))
{
write!(
f,
"Groups {} are incompatible with the declared conflicts: {{{set}}}",
conjunction(
self.conflicts
.iter()
.map(|conflict| match conflict {
ConflictPackage::Group(ref group) if self.dev.default(group) =>
format!("`{group}` (enabled by default)"),
ConflictPackage::Group(ref group) => format!("`{group}`"),
ConflictPackage::Extra(..) => unreachable!(),
})
.collect()
)
)
} else {
write!(
f,
"{} are incompatible with the declared conflicts: {{{set}}}",
conjunction(
self.conflicts
.iter()
.enumerate()
.map(|(i, conflict)| {
let conflict = match conflict {
ConflictPackage::Extra(ref extra) => format!("extra `{extra}`"),
ConflictPackage::Group(ref group) if self.dev.default(group) => {
format!("group `{group}` (enabled by default)")
}
ConflictPackage::Group(ref group) => format!("group `{group}`"),
};
(i == 0).then(|| capitalize(&conflict)).unwrap_or(conflict)
})
.collect()
)
)
}
}
}

Expand Down
29 changes: 1 addition & 28 deletions crates/uv/src/commands/tool/upgrade.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use crate::commands::project::{
};
use crate::commands::reporters::PythonDownloadReporter;
use crate::commands::tool::common::remove_entrypoints;
use crate::commands::{tool::common::install_executables, ExitStatus, SharedState};
use crate::commands::{conjunction, tool::common::install_executables, ExitStatus, SharedState};
use crate::printer::Printer;
use crate::settings::ResolverInstallerSettings;

Expand Down Expand Up @@ -356,30 +356,3 @@ async fn upgrade_tool(

Ok(outcome)
}

/// Given a list of names, return a conjunction of the names (e.g., "Alice, Bob and Charlie").
fn conjunction(names: Vec<String>) -> String {
let mut names = names.into_iter();
let first = names.next();
let last = names.next_back();
match (first, last) {
(Some(first), Some(last)) => {
let mut result = first;
let mut comma = false;
for name in names {
result.push_str(", ");
result.push_str(&name);
comma = true;
}
if comma {
result.push_str(", and ");
} else {
result.push_str(" and ");
}
result.push_str(&last);
result
}
(Some(first), None) => first,
_ => String::new(),
}
}
18 changes: 9 additions & 9 deletions crates/uv/tests/it/lock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2361,7 +2361,7 @@ fn lock_conflicting_extra_basic() -> Result<()> {
----- stdout -----

----- stderr -----
error: extra `project1`, extra `project2` are incompatible with the declared conflicts: {`project[project1]`, `project[project2]`}
error: Extras `project1` and `project2` are incompatible with the declared conflicts: {`project[project1]`, `project[project2]`}
"###);
// As should exporting them.
uv_snapshot!(context.filters(), context.export().arg("--frozen").arg("--all-extras"), @r###"
Expand All @@ -2370,7 +2370,7 @@ fn lock_conflicting_extra_basic() -> Result<()> {
----- stdout -----

----- stderr -----
error: extra `project1`, extra `project2` are incompatible with the declared conflicts: {`project[project1]`, `project[project2]`}
error: Extras `project1` and `project2` are incompatible with the declared conflicts: {`project[project1]`, `project[project2]`}
"###);

Ok(())
Expand Down Expand Up @@ -2604,7 +2604,7 @@ fn lock_conflicting_extra_multiple_not_conflicting1() -> Result<()> {
----- stdout -----

----- stderr -----
error: extra `project1`, extra `project2` are incompatible with the declared conflicts: {`project[project1]`, `project[project2]`}
error: Extras `project1` and `project2` are incompatible with the declared conflicts: {`project[project1]`, `project[project2]`}
"###);
// project3/project4 conflict!
uv_snapshot!(
Expand All @@ -2616,7 +2616,7 @@ fn lock_conflicting_extra_multiple_not_conflicting1() -> Result<()> {
----- stdout -----

----- stderr -----
error: extra `project3`, extra `project4` are incompatible with the declared conflicts: {`project[project3]`, `project[project4]`}
error: Extras `project3` and `project4` are incompatible with the declared conflicts: {`project[project3]`, `project[project4]`}
"###);
// ... but project1/project3 does not.
uv_snapshot!(
Expand Down Expand Up @@ -3759,7 +3759,7 @@ fn lock_conflicting_group_basic() -> Result<()> {
----- stdout -----

----- stderr -----
error: group `project1`, group `project2` are incompatible with the declared conflicts: {`project:project1`, `project:project2`}
error: Groups `project1` and `project2` are incompatible with the declared conflicts: {`project:project1`, `project:project2`}
"###);

Ok(())
Expand Down Expand Up @@ -3914,7 +3914,7 @@ fn lock_conflicting_group_default() -> Result<()> {
----- stdout -----

----- stderr -----
error: group `project1` (enabled by default), group `project2` are incompatible with the declared conflicts: {`project:project1`, `project:project2`}
error: Groups `project1` (enabled by default) and `project2` are incompatible with the declared conflicts: {`project:project1`, `project:project2`}
"###);

// If the group is explicitly requested, we should still fail, but shouldn't mark it as
Expand All @@ -3925,7 +3925,7 @@ fn lock_conflicting_group_default() -> Result<()> {
----- stdout -----

----- stderr -----
error: group `project1`, group `project2` are incompatible with the declared conflicts: {`project:project1`, `project:project2`}
error: Groups `project1` and `project2` are incompatible with the declared conflicts: {`project:project1`, `project:project2`}
"###);

// If we install via `--all-groups`, we should also avoid marking the group as "enabled by
Expand All @@ -3936,7 +3936,7 @@ fn lock_conflicting_group_default() -> Result<()> {
----- stdout -----

----- stderr -----
error: group `project1`, group `project2` are incompatible with the declared conflicts: {`project:project1`, `project:project2`}
error: Groups `project1` and `project2` are incompatible with the declared conflicts: {`project:project1`, `project:project2`}
"###);

// Disabling the default group should succeed.
Expand Down Expand Up @@ -4153,7 +4153,7 @@ fn lock_conflicting_mixed() -> Result<()> {
----- stdout -----

----- stderr -----
error: group `project1`, extra `project2` are incompatible with the declared conflicts: {`project:project1`, `project[project2]`}
error: Group `project1` and extra `project2` are incompatible with the declared conflicts: {`project:project1`, `project[project2]`}
"###);

Ok(())
Expand Down

0 comments on commit b2bad8d

Please sign in to comment.