-
Notifications
You must be signed in to change notification settings - Fork 635
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
Support PEP 723 scripts in uv add
and uv remove
#5995
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, look forward to reviewing, this feature is gonna be amazing.
If you haven’t already (sorry, on phone), can you ensure there’s test coverage for:
- Adding a dep with an existing script tag
- Adding a dep without an existing script tag (with a shebang present, with a docstring present, etc.)
- Removing the last dep in an existing script tag (I think it’s fine to leave the script tag rather than removing it entirely, even if the dep list is empty).
|
Err(err) => return Err(err.into()), | ||
}; | ||
let (requirement, source) = match dependency_destination { | ||
DependencyDestination::Script(_) => (pep508_rs::Requirement::from(requirement), None), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These actually do support sources now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for sources and a test for this
crates/uv-scripts/src/lib.rs
Outdated
@@ -163,11 +157,13 @@ fn extract_script_tag(contents: &[u8]) -> Result<Option<String>, Pep723Error> { | |||
|
|||
// Otherwise, the line _must_ start with ` `. | |||
let Some(line) = line.strip_prefix(' ') else { | |||
python_script.push(line); | |||
python_script.extend(lines); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we break, doesn't the python_script
get truncated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python_script
gets all of the remaining lines for the files before we break so it should contain the rest of the script file.
crates/uv-scripts/src/lib.rs
Outdated
/// Given the contents of a Python file, extract the `script` metadata block, with leading comment | ||
/// hashes removed. | ||
/// hashes removed and the python script. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the "python script"? Can you show an example in the docstring here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By this I meant the python code in the rest of the script, I've added an example and some further explanation for this.
@@ -2509,6 +2513,9 @@ pub struct RemoveArgs { | |||
#[arg(long, conflicts_with = "isolated")] | |||
pub package: Option<PackageName>, | |||
|
|||
/// Specifies the Python script where the dependency will be removed. | |||
#[arg(long)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think --script
should be marked as conflicting with a bunch of other options, like: --locked
, --frozen
, --dev
, etc. Or, we can warn in add
and remove
if those are set, to indicate that they're ignored, like in #5977. Fine to just follow that pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a warning for the other options, let me know if I missed something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I marked --dev
and --optional
as conflicting, since those should truly error. The rest are more "redundant".
Pumped for this! |
Wow so exciting! |
Can you expand on this a little about what we want to support here? Just to clarify, do we want to support the following use case with uv add? Suppose the script file contains this code: import requests
from rich.pretty import pprint
resp = requests.get("https://peps.python.org/api/peps.json")
data = resp.json()
pprint([(k, v["title"]) for k, v in data.items()][:10]) If the user runs: uv add rich requests --script example.py Do we intend for uv add to transform the script by adding a metadata block like this at the top? # /// script
# requires-python = ">=3.11"
# dependencies = [
# "requests",
# "rich",
# ]
# ///
import requests
from rich.pretty import pprint
resp = requests.get("https://peps.python.org/api/peps.json")
data = resp.json()
pprint([(k, v["title"]) for k, v in data.items()][:10]) |
Yeah that’s what I was thinking. My guess is it needs to appear after any shebang though (but maybe before any docstrings?) |
Thank you, that makes sense. I'll look into adding that. |
This should now support adding a dependency even when there is no existing script tag in the file. |
Great! I assume that means ready to review, I'll go through it now. |
0c31c2d
to
079915c
Compare
079915c
to
40cba44
Compare
Great, thank you! |
be2d747
to
dc37649
Compare
dc37649
to
3096e0f
Compare
Summary
Resolves #4667
Test Plan
cargo test