Return error when running uvx with a .py script #11623
Conversation
jtfmumm
force-pushed
the
jtfm/uvx-run-hint
branch
from
d477bb2 to
7a5208f
Compare
crates/uv/src/commands/tool/run.rs
Outdated
| .extension() | ||
| .is_some_and(|ext| ext.eq_ignore_ascii_case("py")) | ||
| { | ||
| if possible_path.try_exists()? { |
There was a problem hiding this comment.
Should we error if we can't read the path? (earnestly not sure here)
There was a problem hiding this comment.
It's a good question. We could treat that as the "doesn't exist case" if we'd prefer.
crates/uv/src/commands/tool/run.rs
Outdated
| )); | ||
| } | ||
| return Err(anyhow::anyhow!( | ||
| "It looks like you are trying to run a script that doesn't exist. \n\n{}{} Did you mean `uvx {}`?", |
There was a problem hiding this comment.
It seems nice to include the script name, maybe like
It looks like you're trying to run a script (
{}) but it does not exist.
Not in love with that, but don't have a better idea.
There was a problem hiding this comment.
In the "Did you mean" message, should we clarify what uvx <name> would do? Like "Did you mean to run a tool with uvx {}"?
I'm not sure if I prefer it, but wanted to put it out there.
There was a problem hiding this comment.
Yeah, I think adding that context is helpful
crates/uv/src/commands/tool/run.rs
Outdated
| @@ -104,6 +105,26 @@ pub(crate) async fn run( | |||
| return Err(anyhow::anyhow!("Tool command could not be parsed as UTF-8 string. Use `--from` to specify the package name.")); | |||
| }; | |||
|
|
|||
| let possible_path = Path::new(target); | |||
There was a problem hiding this comment.
If --from is provided, e.g, uvx --from <package> foo.py we should probably not perform this check as they've opted-in to loading a specific source.
We probably want to have this logic for uvx --from foo.py <command> though? We can't give a good suggestion though since we don't support like uv run --with-requirements foo.py <command> yet — see #6542
There was a problem hiding this comment.
Updated to reflect your suggestion
crates/uv/src/commands/tool/run.rs
Outdated
| .is_some_and(|ext| ext.eq_ignore_ascii_case("py") || ext.eq_ignore_ascii_case("pyw")) | ||
| { | ||
| return Err(anyhow::anyhow!( | ||
| "It looks you have passed a script instead of a package into `--from`" |
There was a problem hiding this comment.
Why not use the same suggestion as below? (but with --from <package>)
There was a problem hiding this comment.
We can't suggest uv run ... but we can suggest uvx --from script-py ... instead
crates/uv/src/commands/tool/run.rs
Outdated
| )) | ||
| } else { | ||
| Err(anyhow::anyhow!( | ||
| "It looks like you are trying to run a script that doesn't exist. \n\n{}{} Did you mean to run a tool with `uvx {}`?", |
There was a problem hiding this comment.
Sorry I missed this initially, you need to use invocation_source here instead of hard-coding uvx because that's an alias for uv tool run (and we want to match what the user used)
|
This looks good to me, thanks for addressing the feedback. I want to do one final pass on the messaging, since this is ostensibly breaking (though I really hope nobody is relying on this behavior). I'll do that on Monday. |
If we see
uvx script.py, we exit early, giving a hint to useuv run script.pyif the script exists. If it does not exist, we suggest runninguv runwith a normalized package name.This PR includes a snapshot test for each of these scenarios.
An alternative approach would be to wait until we encounter an error, and then add the hint. But if there happens to be a malicious package called
script-py, this would be run unintentionally (a point raised by @zanieb).Closes #10784