Skip to content

Commit

Permalink
perf: avoid retrieving possible_values unless they're used
Browse files Browse the repository at this point in the history
In some sophisticated situations, these may be expensive to calculate.
One example might be a '--branch' option accepting any single Git
branch that exists on the remote -- in such a case, the remote would
need to be queried for all possible_values. The cost is ultimately
unavoidable at runtime since this validation has to happen eventually,
but there's no need to pay it when generating help text if
`is_hide_possible_values_set`.

To keep '-h' fast, avoid collecting `possible_values` during '-h'
unless we're actually going to use the values in display.
  • Loading branch information
vermiculus committed Dec 24, 2023
1 parent d092896 commit d281a72
Show file tree
Hide file tree
Showing 3 changed files with 167 additions and 49 deletions.
97 changes: 48 additions & 49 deletions clap_builder/src/output/help_template.rs
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 !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);
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();

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 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);
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/builder/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ mod opts;
mod positionals;
mod posix_compatible;
mod possible_values;
mod possible_values_expensive;
mod propagate_globals;
mod require;
mod subcommands;
Expand Down
118 changes: 118 additions & 0 deletions tests/builder/possible_values_expensive.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
#![cfg(feature = "string")]

use std::{cell::RefCell, rc::Rc};

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

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

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

impl ExpensiveValues {
pub fn new() -> Self {
ExpensiveValues {
iterated: Rc::new(RefCell::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> {
*self.me.iterated.borrow_mut() = true;

self.index += 1;

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

#[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(PossibleValuesParser::new(expensive.clone())),
),
"clap-test -h",
PV_EXPECTED,
false,
);
assert_eq!(*expensive.iterated.borrow(), 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, expensive-value-3]
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(PossibleValuesParser::new(expensive.clone())),
),
"clap-test -h",
PV_EXPECTED,
false,
);
assert_eq!(*expensive.iterated.borrow(), true);
}

0 comments on commit d281a72

Please sign in to comment.