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.

Sign up for GitHub

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

Jump to bottom

perf: avoid retrieving possible_values unless they're used #5267

Merged
merged 1 commit into from
Dec 28, 2023
Merged

perf: avoid retrieving possible_values unless they're used #5267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
115 changes: 58 additions & 57 deletions clap_builder/src/output/help_template.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,57 +684,56 @@ impl<'cmd, 'writer> HelpTemplate<'cmd, 'writer> {
let help_is_empty = help.is_empty();
self.writer.push_styled(&help);
if let Some(arg) = arg {
const DASH_SPACE: usize = "- ".len();
let possible_vals = arg.get_possible_values();
if !possible_vals.is_empty()
&& !arg.is_hide_possible_values_set()
&& self.use_long_pv(arg)
{
debug!("HelpTemplate::help: Found possible vals...{possible_vals:?}");
let longest = possible_vals
.iter()
.filter(|f| !f.is_hide_set())
.map(|f| display_width(f.get_name()))
.max()
.expect("Only called with possible value");

let spaces = spaces + TAB_WIDTH - DASH_SPACE;
let trailing_indent = spaces + DASH_SPACE;
let trailing_indent = self.get_spaces(trailing_indent);
if !arg.is_hide_possible_values_set() && self.use_long_pv(arg) {
const DASH_SPACE: usize = "- ".len();
let possible_vals = arg.get_possible_values();
if !possible_vals.is_empty() {
debug!("HelpTemplate::help: Found possible vals...{possible_vals:?}");
let longest = possible_vals
.iter()
.filter(|f| !f.is_hide_set())
.map(|f| display_width(f.get_name()))
.max()
.expect("Only called with possible value");

let spaces = spaces + TAB_WIDTH - DASH_SPACE;
let trailing_indent = spaces + DASH_SPACE;
let trailing_indent = self.get_spaces(trailing_indent);

if !help_is_empty {
let _ = write!(self.writer, "\n\n{:spaces$}", "");
}
self.writer.push_str("Possible values:");
for pv in possible_vals.iter().filter(|pv| !pv.is_hide_set()) {
let name = pv.get_name();

if !help_is_empty {
let _ = write!(self.writer, "\n\n{:spaces$}", "");
}
self.writer.push_str("Possible values:");
for pv in possible_vals.iter().filter(|pv| !pv.is_hide_set()) {
let name = pv.get_name();
let mut descr = StyledStr::new();
let _ = write!(
&mut descr,
"{}{name}{}",
literal.render(),
literal.render_reset()
);
if let Some(help) = pv.get_help() {
debug!("HelpTemplate::help: Possible Value help");
// To align help messages
let padding = longest - display_width(name);
let _ = write!(&mut descr, ": {:padding$}", "");
descr.push_styled(help);
}

let mut descr = StyledStr::new();
let _ = write!(
&mut descr,
"{}{name}{}",
literal.render(),
literal.render_reset()
);
if let Some(help) = pv.get_help() {
debug!("HelpTemplate::help: Possible Value help");
// To align help messages
let padding = longest - display_width(name);
let _ = write!(&mut descr, ": {:padding$}", "");
descr.push_styled(help);
let avail_chars = if self.term_w > trailing_indent.len() {
self.term_w - trailing_indent.len()
} else {
usize::MAX
};
descr.replace_newline_var();
descr.wrap(avail_chars);
descr.indent("", &trailing_indent);

let _ = write!(self.writer, "\n{:spaces$}- ", "",);
self.writer.push_styled(&descr);
}

let avail_chars = if self.term_w > trailing_indent.len() {
self.term_w - trailing_indent.len()
} else {
usize::MAX
};
descr.replace_newline_var();
descr.wrap(avail_chars);
descr.indent("", &trailing_indent);

let _ = write!(self.writer, "\n{:spaces$}- ", "",);
self.writer.push_styled(&descr);
}
}
}
Expand Down Expand Up @@ -844,17 +843,19 @@ impl<'cmd, 'writer> HelpTemplate<'cmd, 'writer> {
spec_vals.push(format!("[short aliases: {als}]"));
}

let possible_vals = a.get_possible_values();
if !possible_vals.is_empty() && !a.is_hide_possible_values_set() && !self.use_long_pv(a) {
debug!("HelpTemplate::spec_vals: Found possible vals...{possible_vals:?}");
if !a.is_hide_possible_values_set() && !self.use_long_pv(a) {
let possible_vals = a.get_possible_values();
if !possible_vals.is_empty() {
debug!("HelpTemplate::spec_vals: Found possible vals...{possible_vals:?}");

let pvs = possible_vals
.iter()
.filter_map(PossibleValue::get_visible_quoted_name)
.collect::<Vec<_>>()
.join(", ");
let pvs = possible_vals
.iter()
.filter_map(PossibleValue::get_visible_quoted_name)
.collect::<Vec<_>>()
.join(", ");

spec_vals.push(format!("[possible values: {pvs}]"));
spec_vals.push(format!("[possible values: {pvs}]"));
}
}
let connector = if self.use_long { "\n" } else { " " };
spec_vals.join(connector)
Expand Down
6 changes: 5 additions & 1 deletion clap_mangen/src/render.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,15 @@ fn option_default_values(opt: &clap::Arg) -> Option<String> {
}

fn get_possible_values(arg: &clap::Arg) -> Option<(Vec<String>, bool)> {
if arg.is_hide_possible_values_set() {
return None;
}

let possibles = &arg.get_possible_values();
let possibles: Vec<&clap::builder::PossibleValue> =
possibles.iter().filter(|pos| !pos.is_hide_set()).collect();

if !(possibles.is_empty() || arg.is_hide_possible_values_set()) {
if !possibles.is_empty() {
return Some(format_possible_values(&possibles));
}
None
Expand Down
142 changes: 142 additions & 0 deletions tests/builder/possible_values.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,3 +472,145 @@ fn ignore_case_multiple_fail() {
assert!(m.is_err());
assert_eq!(m.unwrap_err().kind(), ErrorKind::InvalidValue);
}

#[cfg(feature = "string")]
mod expensive {
use std::sync::{Arc, Mutex};

use clap::{Arg, Command};
use clap_builder::builder::{PossibleValue, PossibleValuesParser, TypedValueParser};

#[cfg(feature = "error-context")]
use super::utils;

#[derive(Clone)]
struct ExpensiveValues {
iterated: Arc<Mutex<bool>>,
}

impl ExpensiveValues {
pub fn new() -> Self {
ExpensiveValues {
iterated: Arc::new(Mutex::new(false)),
}
}
}

impl IntoIterator for ExpensiveValues {
type Item = String;

type IntoIter = ExpensiveValuesIntoIterator;

fn into_iter(self) -> Self::IntoIter {
ExpensiveValuesIntoIterator { me: self, index: 0 }
}
}

struct ExpensiveValuesIntoIterator {
me: ExpensiveValues,
index: usize,
}

impl Iterator for ExpensiveValuesIntoIterator {
type Item = String;
fn next(&mut self) -> Option<String> {
let mut guard = self
.me
.iterated
.lock()
.expect("not working across multiple threads");

*guard = true;
self.index += 1;

if self.index < 3 {
Some(format!("expensive-value-{}", self.index))
} else {
None
}
}
}

impl TypedValueParser for ExpensiveValues {
type Value = String;

fn parse_ref(
&self,
_cmd: &clap_builder::Command,
_arg: Option<&clap_builder::Arg>,
_value: &std::ffi::OsStr,
) -> Result<Self::Value, clap_builder::Error> {
todo!()
}

fn possible_values(&self) -> Option<Box<dyn Iterator<Item = PossibleValue> + '_>> {
Some(Box::new(self.clone().into_iter().map(PossibleValue::from)))
}
}

#[test]
fn no_iterate_when_hidden() {
static PV_EXPECTED: &str = "\
Usage: clap-test [some-cheap-option] [some-expensive-option]

Arguments:
[some-cheap-option] cheap [possible values: some, cheap, values]
[some-expensive-option] expensive

Options:
-h, --help Print help
";
let expensive = ExpensiveValues::new();
utils::assert_output(
Command::new("test")
.arg(
Arg::new("some-cheap-option")
.help("cheap")
.value_parser(PossibleValuesParser::new(["some", "cheap", "values"])),
)
.arg(
Arg::new("some-expensive-option")
.help("expensive")
.hide_possible_values(true)
.value_parser(expensive.clone()),
),
"clap-test -h",
PV_EXPECTED,
false,
);
assert_eq!(*expensive.iterated.lock().unwrap(), false);
}

#[test]
fn iterate_when_displayed() {
static PV_EXPECTED: &str = "\
Usage: clap-test [some-cheap-option] [some-expensive-option]

Arguments:
[some-cheap-option] cheap [possible values: some, cheap, values]
[some-expensive-option] expensive [possible values: expensive-value-1, expensive-value-2]

Options:
-h, --help Print help
";
let expensive = ExpensiveValues::new();
utils::assert_output(
Command::new("test")
.arg(
Arg::new("some-cheap-option")
.help("cheap")
.value_parser(PossibleValuesParser::new(["some", "cheap", "values"])),
)
.arg(
Arg::new("some-expensive-option")
.help("expensive")
.hide_possible_values(false)
.value_parser(expensive.clone()),
),
"clap-test -h",
PV_EXPECTED,
false,
);
assert_eq!(*expensive.iterated.lock().unwrap(), true);
}
}