Skip to content

Add various grammar changes to conflict error messages #9369

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 1 commit into from
Nov 22, 2024
Merged

Add various grammar changes to conflict error messages #9369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 7 additions & 6 deletions crates/uv-python/src/discovery.rs
View file
Open in desktop
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
Open in desktop
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 {
Copy link
Copy Markdown
Member

@zanieb zanieb Nov 22, 2024

Choose a reason for hiding this comment

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

Hilariously, there's also

fn conjunction(items: &[&str]) -> String {
    match items.len() {
        1 => items[0].to_string(),
        2 => format!("{} or {}", items[0], items[1]),
        _ => {
            let last = items.last().unwrap();
            format!(
                "{}, or {}",
                items.iter().take(items.len() - 1).join(", "),
                last
            )
        }
    }
}

Copy link
Copy Markdown
Member Author

@charliermarsh charliermarsh Nov 22, 2024

Choose a reason for hiding this comment

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

Lol. Do we like that impl more?

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 ");
Copy link
Copy Markdown
Member

@BurntSushi BurntSushi Nov 22, 2024

Choose a reason for hiding this comment

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

Boooo Oxford comma!

} 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
Open in desktop
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
Open in desktop
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
Open in desktop
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]`}
Copy link
Copy Markdown
Member Author

@charliermarsh charliermarsh Nov 22, 2024

Choose a reason for hiding this comment

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

Capitalizing here feels a bit off but we do capitalize everywhere else...

"###);
// 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