-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add --show-with
to uv tool list
to list packages included by --with
#13264
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
Changes from 5 commits
ed45d7d
8868b89
813ad7a
8590b8d
d7f2528
26d0391
a14844c
01a4812
0b886d6
03c9974
eb874fc
d1d634b
55af20c
ebaa15c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ use crate::printer::Printer; | |
pub(crate) async fn list( | ||
show_paths: bool, | ||
show_version_specifiers: bool, | ||
show_with: bool, | ||
cache: &Cache, | ||
printer: Printer, | ||
) -> Result<ExitStatus> { | ||
|
@@ -80,18 +81,41 @@ pub(crate) async fn list( | |
String::new() | ||
}; | ||
|
||
let extra_requirements = if show_with { | ||
gaardhus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let extra_requirements = tool | ||
.requirements() | ||
.iter() | ||
.filter(|req| req.name != name) | ||
.map(|req| { | ||
if !req.source.to_string().is_empty() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the requirement source display ever empty? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just copied the filtering from the code block above, but removing any if them doesn't change the output or the status of the tests? So I guess either remove them, or if the are required ideally add test cases to capture that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's possible for it to be empty from looking at the implementation 🤷♀️ not sure why it's that way. |
||
format!("{}{}", req.name, req.source) | ||
} else { | ||
req.name.to_string() | ||
} | ||
}) | ||
.filter(|s| !s.is_empty()) | ||
.join(", "); | ||
if extra_requirements.is_empty() { | ||
String::new() | ||
} else { | ||
format!(" [with: {extra_requirements}]") | ||
} | ||
} else { | ||
String::new() | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could rewrite this entire block as something like let extra_requirements = (show_with && !tool.requirements().is_empty())
.then(|| {
let requirements = tool
.requirements()
.iter()
// Exclude the package itself
.filter(|req| req.name != name)
.map(|req| format!("{}{}", req.name, req.source))
.join(", ");
format!(" [with: {requirements}]")
})
.unwrap_or_default(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay that broke some stuff, need to add a check that requirements exists, I guess since installing without any --with requirements still returns the original package, and hence returns an empty There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm super new to rust, so sorry if there is something I don't ace. Since the requirements array may be empty, I think we do need an if else clause here before inserting the values? So currently working implementation would be let with_requirements = (show_with && !tool.requirements().is_empty())
.then(|| {
let requirements = tool
.requirements()
.iter()
.filter(|req| req.name != name)
.map(|req| format!("{}{}", req.name, req.source))
.join(", ");
if requirements.is_empty() {
String::new()
} else {
format!(" [with: {requirements}]")
}
})
.unwrap_or_default(); My initial implementation was based on the version_specifier parsing on the lines above. So if you prefer unwrap_or_default should I also change that chunk to mirror that, ie. let version_specifier = show_version_specifiers
.then(|| {
let specifiers = tool
.requirements()
.iter()
.filter(|req| req.name == name)
.map(|req| req.source.to_string())
.filter(|s| !s.is_empty())
.join(", ");
if specifiers.is_empty() {
String::new()
} else {
format!(" [required: {specifiers}]")
}
})
.unwrap_or_default(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could just do The prettier way is like.. let extra_requirements = show_with
.then(|| {
tool.requirements()
.iter()
// Exclude the package itself
.filter(|req| req.name != name)
.peekable()
})
// Omit the `[with: ...]` if there are no extra requirements
.filter(|requirements| requirements.peek().is_some())
.map(|requirements| {
let requirements = requirements
.iter()
.map(|req| format!("{}{}", req.name, req.source))
.join(", ");
format!(" [with: {requirements}]")
})
.unwrap_or_default(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could just do The prettier way is like.. let extra_requirements = show_with
.then(|| {
tool.requirements()
.iter()
// Exclude the package itself
.filter(|req| req.name != name)
.peekable()
})
// Omit the `[with: ...]` if there are no extra requirements
.filter(|requirements| requirements.peek().is_some())
.map(|requirements| {
let requirements = requirements
.iter()
.map(|req| format!("{}{}", req.name, req.source))
.join(", ");
format!(" [with: {requirements}]")
})
.unwrap_or_default(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for your suggestions and detailed review! I couldn't make the peekable work, the compiler kept throwing errors about mutability. So I tried reimplementing and unifying the two functions like this let tool_requirements = (show_version_specifiers | show_with)
.then(|| tool.requirements())
.unwrap_or_default();
let version_specifier = show_version_specifiers
.then(|| {
let specifiers = tool_requirements
.iter()
.filter(|req| req.name == name)
.map(|req| req.source.to_string())
.filter(|s| !s.is_empty())
.join(", ");
(!specifiers.is_empty())
.then(|| format!(" [required: {specifiers}]"))
.unwrap_or_default()
})
.unwrap_or_default();
let with_requirements = (show_with && tool_requirements.len() > 1)
.then(|| {
let requirements = tool_requirements
.iter()
.filter(|req| req.name != name)
.map(|req| format!("{}{}", req.name, req.source))
.join(", ");
format!(" [with: {requirements}]")
})
.unwrap_or_default(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah sorry about that, the mutable reference thing is annoying... I'm not sure if there's a fix for that (but I'll ask someone who knows more than me). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Rust's idioms around options take a while to learn, but they are nice once you get the hang of it) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then they become let version_specifier = show_version_specifiers
.then(|| {
tool.requirements()
.iter()
.filter(|req| req.name == name)
.map(|req| req.source.to_string())
.filter(|s| !s.is_empty())
.peekable()
})
.take_if(|specifiers| specifiers.peek().is_some())
.map(|mut specifiers| {
let specifiers = specifiers.join(", ");
format!(" [required: {specifiers}]")
})
.unwrap_or_default();
let with_requirements = show_with
.then(|| {
tool.requirements()
.iter()
.filter(|req| req.name != name)
.peekable()
})
.take_if(|requirements| requirements.peek().is_some())
.map(|requirements| {
let requirements = requirements
.map(|req| format!("{}{}", req.name, req.source))
.join(", ");
format!(" [with: {requirements}]")
})
.unwrap_or_default(); if I'm not mistaken? |
||
|
||
if show_paths { | ||
writeln!( | ||
printer.stdout(), | ||
"{} ({})", | ||
format!("{name} v{version}{version_specifier}").bold(), | ||
format!("{name} v{version}{version_specifier}{extra_requirements}").bold(), | ||
installed_tools.tool_dir(&name).simplified_display().cyan(), | ||
)?; | ||
} else { | ||
writeln!( | ||
printer.stdout(), | ||
"{}", | ||
format!("{name} v{version}{version_specifier}").bold() | ||
format!("{name} v{version}{version_specifier}{extra_requirements}").bold() | ||
)?; | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.