Skip to content
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.

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

Add some decoration to tool CLI #4865

Merged
merged 1 commit into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 24 additions & 14 deletions crates/uv/src/commands/tool/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::str::FromStr;

use anyhow::{bail, Context, Result};
use itertools::Itertools;
use owo_colors::OwoColorize;
use tracing::debug;

use distribution_types::Name;
Expand Down Expand Up @@ -82,7 +83,7 @@ pub(crate) async fn install(
// Parse the positional name. If the user provided more than a package name, it's an error
// (e.g., `uv install foo==1.0 --from foo`).
let Ok(package) = PackageName::from_str(&package) else {
bail!("Package requirement `{from}` provided with `--from` conflicts with install request `{package}`")
bail!("Package requirement (`{from}`) provided with `--from` conflicts with install request (`{package}`)")
};

let from_requirement = resolve_requirements(
Expand All @@ -105,7 +106,7 @@ pub(crate) async fn install(
if from_requirement.name != package {
// Determine if it's an entirely different package (e.g., `uv install foo --from bar`).
bail!(
"Package name `{}` provided with `--from` does not match install request `{}`",
"Package name (`{}`) provided with `--from` does not match install request (`{}`)",
from_requirement.name,
package
);
Expand Down Expand Up @@ -160,12 +161,12 @@ pub(crate) async fn install(
.filter(|environment| {
python_request.as_ref().map_or(true, |python_request| {
if python_request.satisfied(environment.interpreter(), cache) {
debug!("Found existing environment for tool `{}`", from.name);
debug!("Found existing environment for `{}`", from.name);
true
} else {
let _ = writeln!(
printer.stderr(),
"Existing environment for `{}` does not satisfy the requested Python interpreter: `{}`",
"Existing environment for `{}` does not satisfy the requested Python interpreter (`{}`)",
from.name,
python_request
);
Expand All @@ -187,7 +188,7 @@ pub(crate) async fn install(
// And the user didn't request a reinstall or upgrade...
if !force && settings.reinstall.is_none() && settings.upgrade.is_none() {
// We're done.
writeln!(printer.stderr(), "Tool `{from}` is already installed")?;
writeln!(printer.stderr(), "`{from}` is already installed")?;
return Ok(ExitStatus::Failure);
}
}
Expand Down Expand Up @@ -269,7 +270,7 @@ pub(crate) async fn install(
.context("Failed to create executable directory")?;

debug!(
"Installing tool entry points into {}",
"Installing tool executables into: {}",
executable_directory.user_display()
);

Expand Down Expand Up @@ -298,7 +299,7 @@ pub(crate) async fn install(
// Clean up the environment we just created
installed_tools.remove_environment(&from.name)?;

bail!("No entry points found for tool `{}`", from.name);
bail!("No executables found for `{}`", from.name);
}

// Check if they exist, before installing
Expand All @@ -311,7 +312,7 @@ pub(crate) async fn install(
// will _not_ remove existing entry points when they are not managed by uv.
if force || reinstall_entry_points {
for (name, _, target) in existing_entry_points {
debug!("Removing existing entry point `{name}`");
debug!("Removing existing executable: `{name}`");
fs_err::remove_file(target)?;
}
} else if existing_entry_points.peek().is_some() {
Expand All @@ -328,25 +329,34 @@ pub(crate) async fn install(
("s", "exist")
};
bail!(
"Entry point{s} for tool already {exists}: {} (use `--force` to overwrite)",
existing_entry_points.iter().join(", ")
"Executable{s} already {exists}: {} (use `--force` to overwrite)",
existing_entry_points
.iter()
.map(|name| name.bold())
.join(", ")
)
}

for (name, source_path, target_path) in &target_entry_points {
debug!("Installing `{name}`");
debug!("Installing executable: `{name}`");
#[cfg(unix)]
replace_symlink(source_path, target_path).context("Failed to install entrypoint")?;
replace_symlink(source_path, target_path).context("Failed to install executable")?;
#[cfg(windows)]
fs_err::copy(source_path, target_path).context("Failed to install entrypoint")?;
}

let s = if target_entry_points.len() == 1 {
""
} else {
"s"
};
writeln!(
printer.stderr(),
"Installed: {}",
"Installed {} executable{s}: {}",
target_entry_points.len(),
target_entry_points
.iter()
.map(|(name, _, _)| name)
.map(|(name, _, _)| name.bold())
.join(", ")
)?;

Expand Down
5 changes: 3 additions & 2 deletions crates/uv/src/commands/tool/list.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::fmt::Write;

use anyhow::Result;
use owo_colors::OwoColorize;

use uv_cache::Cache;
use uv_configuration::PreviewMode;
Expand Down Expand Up @@ -40,11 +41,11 @@ pub(crate) async fn list(
}
};

writeln!(printer.stdout(), "{name} v{version}")?;
writeln!(printer.stdout(), "{}", format!("{name} v{version}").bold())?;

// Output tool entrypoints
for entrypoint in tool.entrypoints() {
writeln!(printer.stdout(), " {}", &entrypoint.name)?;
writeln!(printer.stdout(), "- {}", &entrypoint.name)?;
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/uv/src/commands/tool/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ fn parse_target(target: &OsString) -> Result<(Cow<OsString>, Cow<str>)> {

// e.g. ignore `git+https://github.com/uv/uv.git@main`
if PackageName::from_str(name).is_err() {
debug!("Ignoring non-package name `{}` in command", name);
debug!("Ignoring non-package name `{name}` in command");
return Ok((Cow::Borrowed(target), Cow::Borrowed(target_str)));
}

Expand All @@ -301,6 +301,6 @@ fn parse_target(target: &OsString) -> Result<(Cow<OsString>, Cow<str>)> {
}

// e.g. `uv@invalid`, warn and treat the whole thing as the command
debug!("Ignoring invalid version request `{}` in command", version);
debug!("Ignoring invalid version request `{version}` in command");
Ok((Cow::Borrowed(target), Cow::Borrowed(target_str)))
}
15 changes: 9 additions & 6 deletions crates/uv/src/commands/tool/uninstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::fmt::Write;

use anyhow::{bail, Result};
use itertools::Itertools;
use owo_colors::OwoColorize;
use tracing::debug;

use uv_configuration::PreviewMode;
Expand Down Expand Up @@ -30,12 +31,12 @@ pub(crate) async fn uninstall(
Ok(()) => {
writeln!(
printer.stderr(),
"Removed dangling environment for tool: `{name}` (missing receipt)"
"Removed dangling environment for `{name}`"
)?;
return Ok(ExitStatus::Success);
}
Err(uv_tool::Error::IO(err)) if err.kind() == std::io::ErrorKind::NotFound => {
bail!("Tool `{name}` is not installed");
bail!("`{name}` is not installed");
}
Err(err) => {
return Err(err.into());
Expand All @@ -50,14 +51,14 @@ pub(crate) async fn uninstall(
let entrypoints = receipt.entrypoints();
for entrypoint in entrypoints {
debug!(
"Removing entrypoint: {}",
"Removing executable: {}",
entrypoint.install_path.user_display()
);
match fs_err::tokio::remove_file(&entrypoint.install_path).await {
Ok(()) => {}
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
debug!(
"Entrypoint not found: {}",
"Executable not found: {}",
entrypoint.install_path.user_display()
);
}
Expand All @@ -67,12 +68,14 @@ pub(crate) async fn uninstall(
}
}

let s = if entrypoints.len() == 1 { "" } else { "s" };
writeln!(
printer.stderr(),
"Uninstalled: {}",
"Uninstalled {} executable{s}: {}",
Copy link
Member Author

Choose a reason for hiding this comment

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

Are you opposed to using "executable" for user-facing copy?

Copy link
Member

Choose a reason for hiding this comment

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

It feels a little weird, I don't have any great alternatives though. "tool entry points"? eh.

Copy link
Member

Choose a reason for hiding this comment

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

We use tool entry points everywhere else in this output, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

But entry point is not as general... We also support data scripts which aren't entry points. I feel like entry point is sort of an implementation detail.

Copy link
Member

Choose a reason for hiding this comment

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

Fair. Why not just "commands" then?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have strong feelings here.

Copy link
Member

Choose a reason for hiding this comment

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

Cargo uses executable, e.g.

   Replacing /Users/zb/.cargo/bin/cargo-nextest
    Replaced package `cargo-nextest v0.9.70` with `cargo-nextest v0.9.72` (executable `cargo-nextest`)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'm gonna run with executable. I can change other user-facing copy too for consistency.

entrypoints.len(),
entrypoints
.iter()
.map(|entrypoint| &entrypoint.name)
.map(|entrypoint| entrypoint.name.bold())
.join(", ")
)?;

Expand Down
6 changes: 3 additions & 3 deletions crates/uv/tests/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@ pub struct TestContext {
/// The Python version used for the virtual environment, if any.
pub python_version: Option<PythonVersion>,

// All the Python versions available during this test context.
/// All the Python versions available during this test context.
pub python_versions: Vec<(PythonVersion, PathBuf)>,

// Standard filters for this test context
/// Standard filters for this test context.
filters: Vec<(String, String)>,
}

Expand Down Expand Up @@ -120,7 +120,7 @@ impl TestContext {
#[must_use]
pub fn with_filtered_exe_suffix(mut self) -> Self {
self.filters
.push((std::env::consts::EXE_SUFFIX.to_string(), String::new()));
.push((regex::escape(env::consts::EXE_SUFFIX), String::new()));
self
}

Expand Down
Loading
Loading