Skip to content
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
86 changes: 39 additions & 47 deletions crates/uv/src/commands/tool/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,11 @@ pub(crate) async fn run(
cache: Cache,
printer: Printer,
) -> anyhow::Result<ExitStatus> {
// Treat empty command similar to `uv tool list`, list available tools.
let Some(command) = command else {
match list_available_tools(invocation_source, &cache, printer).await {
// It is a failure because user misses a required tool name.
Ok(()) => return Ok(ExitStatus::Error),
Err(err) => return Err(err),
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Handling this case with ? instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Definitely, way cleaner 🙈

};
// When a command isn't provided, we'll show a brief help including available tools
show_help(invocation_source, &cache, printer).await?;
// Exit as Clap would after displaying help
return Ok(ExitStatus::Error);
};

let (target, args) = command.split();
Expand Down Expand Up @@ -267,71 +265,65 @@ fn get_entrypoints(
/// Display a list of tools that provide the executable.
///
/// If there is no package providing the executable, we will display a message to how to install a package.
async fn list_available_tools(
async fn show_help(
invocation_source: ToolRunCommand,
cache: &Cache,
printer: Printer,
) -> anyhow::Result<()> {
let help = format!(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

"See `{}` for more information.",
format!("{invocation_source} --help").bold()
);

writeln!(
printer.stdout(),
"Provide a command to invoke with `{invocation_source} <command>` \
or `{invocation_source} --from <package> <command>`.\n"
"Provide a command to run with `{}`.\n",
format!("{invocation_source} <command>").bold()
)?;

let installed_tools = InstalledTools::from_settings()?;
let no_tools_installed_msg =
"No tools installed. See `uv tool install --help` for more information.";
Comment on lines -282 to -283
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After playing with this a bit more, I was worried users would feel like they had to use uv tool install before they could use uv tool run which isn't the case so I dropped this message.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

let _lock = match installed_tools.lock().await {
Ok(lock) => lock,
Err(uv_tool::Error::Io(err)) if err.kind() == std::io::ErrorKind::NotFound => {
writeln!(printer.stdout(), "{no_tools_installed_msg}")?;
writeln!(printer.stdout(), "{help}")?;
return Ok(());
}
Err(err) => return Err(err.into()),
};

let mut tools = installed_tools.tools()?.into_iter().collect::<Vec<_>>();
tools.sort_by_key(|(name, _)| name.clone());
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Instead of using sort_by_key I use sorted_by to avoid the extra clone.


let tools = installed_tools
.tools()?
.into_iter()
// Skip invalid tools
.filter_map(|(name, tool)| {
tool.ok().and_then(|_| {
installed_tools
.version(&name, cache)
.ok()
.map(|version| (name, version))
})
})
Comment on lines +297 to +305
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I do all the filtering here, so we don't need to use a buf: String later.

.sorted_by(|(name1, ..), (name2, ..)| name1.cmp(name2))
.collect::<Vec<_>>();

// No tools installed or they're all malformed
if tools.is_empty() {
writeln!(printer.stdout(), "{no_tools_installed_msg}")?;
writeln!(printer.stdout(), "{help}")?;
return Ok(());
}

let mut buf = String::new();
for (name, tool) in tools {
// Skip invalid tools.
let Ok(tool) = tool else {
continue;
};

// Output tool name and version.
let Ok(version) = installed_tools.version(&name, cache) else {
continue;
};
writeln!(buf, "{}", format!("{name} v{version}").bold())?;

// Output tool entrypoints.
for entrypoint in tool.entrypoints() {
writeln!(buf, "- {}", entrypoint.name)?;
}
// Display the tools
writeln!(printer.stdout(), "The following tools are installed:\n")?;
for (name, version) in tools {
writeln!(
printer.stdout(),
"- {} v{version}",
format!("{name}").bold()
)?;
}
Comment on lines 314 to 323
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I dropped the entrypoints, mostly to simplify the output and avoid confusing around --from usage which is a bit more advanced and hard to spell here.


// Installed tools were malformed or failed fetching versions.
if buf.is_empty() {
writeln!(printer.stderr(), "{no_tools_installed_msg}")?;
return Ok(());
}
writeln!(printer.stdout(), "\n{help}")?;

writeln!(
printer.stdout(),
"The following tools are already installed:\n"
)?;
writeln!(printer.stdout(), "{buf}")?;
writeln!(
printer.stdout(),
"See `{invocation_source} --help` for more information."
)?;
Ok(())
}

Expand Down
12 changes: 5 additions & 7 deletions crates/uv/tests/tool_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -753,9 +753,9 @@ fn tool_run_list_installed() {
success: false
exit_code: 2
----- stdout -----
Provide a command to invoke with `uv tool run <command>` or `uv tool run --from <package> <command>`.
Provide a command to run with `uv tool run <command>`.

No tools installed. See `uv tool install --help` for more information.
See `uv tool run --help` for more information.

----- stderr -----
"###);
Expand All @@ -776,13 +776,11 @@ fn tool_run_list_installed() {
success: false
exit_code: 2
----- stdout -----
Provide a command to invoke with `uv tool run <command>` or `uv tool run --from <package> <command>`.
Provide a command to run with `uv tool run <command>`.

The following tools are already installed:
The following tools are installed:

black v24.2.0
- black
- blackd
- black v24.2.0
Comment on lines +781 to +783
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I went back and forth on a newline here. I think it looks better if we have an extra newline separating the list? Relevant for #7687

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The extra line looks way cleaner to me. If you're okay with me putting it here, I will change the other PR.

Copy link
Copy Markdown
Member Author

@zanieb zanieb Sep 27, 2024

Choose a reason for hiding this comment

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

👍 yeah sorry I think I (wrongly) suggested you remove it over there


See `uv tool run --help` for more information.

Expand Down