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

feat: Support visible aliases for possible values #4344

Closed
wants to merge 1 commit into from
Closed

feat: Support visible aliases for possible values #4344

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions clap_mangen/src/render.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,9 +339,13 @@ fn format_possible_values(possibles: &Vec<&clap::builder::PossibleValue>) -> (Ve
if with_help {
for value in possibles {
let val_name = value.get_name();
let mut visible_aliases = value.get_quoted_visible_aliases().join(", ");
if !visible_aliases.is_empty() {
visible_aliases = format!(" (alias: {visible_aliases})");
}
match value.get_help() {
Some(help) => lines.push(format!("{}: {}", val_name, help)),
None => lines.push(val_name.to_string()),
Some(help) => lines.push(format!("{val_name}: {help}{visible_aliases}")),
None => lines.push(format!("{val_name}{visible_aliases}")),
}
}
} else {
Expand Down
5 changes: 4 additions & 1 deletion clap_mangen/tests/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,10 @@ pub fn possible_values_command(name: &'static str) -> clap::Command {
.long("method")
.action(clap::ArgAction::Set)
.value_parser([
PossibleValue::new("fast").help("use the Fast method"),
PossibleValue::new("fast")
.help("use the Fast method")
.visible_alias("quick")
.alias("zippy"),
PossibleValue::new("slow").help("use the slow method"),
PossibleValue::new("normal")
.help("use normal mode")
Expand Down
2 changes: 1 addition & 1 deletion clap_mangen/tests/snapshots/possible_values.bash.roff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ my/-app
/fIPossible values:/fR
.RS 14
.IP /(bu 2
fast: use the Fast method
fast: use the Fast method (alias: quick)
.IP /(bu 2
slow: use the slow method
.RE
Expand Down
8 changes: 7 additions & 1 deletion examples/tutorial_derive/04_01_enum.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ A simple to use, efficient, and full-featured Command Line Argument Parser
Usage: 04_01_enum_derive[EXE] <MODE>

Arguments:
<MODE> What mode to run the program in [possible values: fast, slow]
<MODE> What mode to run the program in [possible values: fast (alias: quick), slow]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel like this is too many layers of parenthetical statements.

We should either flatten them or reserve them for long help.

I lean towards flattening.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage I think it would be confusing to put all the aliases into a single flat list, without indicating what value they're aliases for.

No objections to only showing them in long help, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is more being driven by your needs, I'll defer to your preference until we get wider feedback

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage FWIW, while I have a need for visible aliases, I have no particular preferences regarding how to display them, as long as they're displayed and it's clear what they're an alias for. I do think the brief help may just be too short to reasonably show them.


Options:
-h, --help Print help information
Expand All @@ -17,6 +17,12 @@ Hare
$ 04_01_enum_derive slow
Tortoise

$ 04_01_enum_derive quick
Hare

$ 04_01_enum_derive zippy
Hare

$ 04_01_enum_derive medium
? failed
error: "medium" isn't a valid value for '<MODE>'
Expand Down
1 change: 1 addition & 0 deletions examples/tutorial_derive/04_01_enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ struct Cli {

#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, ValueEnum)]
enum Mode {
#[value(visible_alias("quick"), alias("zippy"))]
Fast,
Slow,
}
Expand Down
72 changes: 62 additions & 10 deletions src/builder/possible_value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use crate::util::eq_ignore_case;
pub struct PossibleValue {
name: Str,
help: Option<StyledStr>,
aliases: Vec<Str>, // (name, visible)
aliases: Vec<(Str, bool)>, // (name, visible)
hide: bool,
}

Expand Down Expand Up @@ -114,7 +114,7 @@ impl PossibleValue {
#[must_use]
pub fn alias(mut self, name: impl IntoResettable<Str>) -> Self {
if let Some(name) = name.into_resettable().into_option() {
self.aliases.push(name);
self.aliases.push((name, false));
} else {
self.aliases.clear();
}
Expand All @@ -133,7 +133,45 @@ impl PossibleValue {
/// ```
#[must_use]
pub fn aliases(mut self, names: impl IntoIterator<Item = impl Into<Str>>) -> Self {
self.aliases.extend(names.into_iter().map(|a| a.into()));
self.aliases
.extend(names.into_iter().map(|a| (a.into(), false)));
self
}

/// Sets a *visible* alias for this argument value.
///
/// # Examples
///
/// ```rust
/// # use clap::builder::PossibleValue;
/// PossibleValue::new("slow")
/// .visible_alias("not-fast")
/// # ;
/// ```
#[must_use]
pub fn visible_alias(mut self, name: impl IntoResettable<Str>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not show aliases in error messages yet.

Whats holding that back?

Copy link
Contributor Author

@joshtriplett joshtriplett Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage Primarily, the amount of additional complexity required to pass groups of aliases through the error-context infrastructure and update all the places creating those. Secondarily, trying to figure out how to handle the "did you mean".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So long as we hide visible aliases in the short help, we can forego them in the errors.

For the suggestion,

  • If we consolidate the two possible value ValueParsers, we could probably switch the "did you mean" up a layer into those which could give greater control over it
  • However, if we suggest a value that was not shown in the error, that might be confusing.

I can go either way

btw can you update the commit with some of these "for now" decisions so its easier for someone to discover them when making changes in the future? People might discover the commit but its then a smaller set of people who might explore the PR comments.

if let Some(name) = name.into_resettable().into_option() {
self.aliases.push((name, true));
} else {
self.aliases.clear();
}
self
}

/// Sets multiple *visible* aliases for this argument value.
///
/// # Examples
///
/// ```rust
/// # use clap::builder::PossibleValue;
/// PossibleValue::new("slow")
/// .visible_aliases(["not-fast", "snake-like"])
/// # ;
/// ```
#[must_use]
pub fn visible_aliases(mut self, names: impl IntoIterator<Item = impl Into<Str>>) -> Self {
self.aliases
.extend(names.into_iter().map(|a| (a.into(), true)));
self
}
}
Expand Down Expand Up @@ -180,21 +218,25 @@ impl PossibleValue {
#[cfg(feature = "help")]
pub(crate) fn get_visible_quoted_name(&self) -> Option<std::borrow::Cow<'_, str>> {
if !self.hide {
Some(if self.name.contains(char::is_whitespace) {
format!("{:?}", self.name).into()
} else {
self.name.as_str().into()
})
Some(quote_if_whitespace(&self.name))
} else {
None
}
}

/// Get the names of all visible aliases, but wrapped in quotes if they contain whitespace
pub fn get_quoted_visible_aliases(&self) -> Vec<std::borrow::Cow<'_, str>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels too specialized to make pub

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage clap_mangen needs to call it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clap_mangen is choosing to format quoted aliases the same way as the --help. I don't see that as a hard requirement to be driving code sharing and rather only couple code together when its driven by requirements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage What would you suggest instead? I don't want to leave them out of the manpages entirely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate what you need

self.aliases
.iter()
.filter_map(|(n, vis)| vis.then(|| quote_if_whitespace(n)))
.collect()
}

/// Returns all valid values of the argument value.
///
/// Namely the name and all aliases.
pub fn get_name_and_aliases(&self) -> impl Iterator<Item = &str> + '_ {
std::iter::once(self.get_name()).chain(self.aliases.iter().map(|s| s.as_str()))
std::iter::once(self.get_name()).chain(self.aliases.iter().map(|(s, _)| s.as_str()))
}

/// Tests if the value is valid for this argument value
Expand All @@ -205,9 +247,10 @@ impl PossibleValue {
///
/// ```rust
/// # use clap::builder::PossibleValue;
/// let arg_value = PossibleValue::new("fast").alias("not-slow");
/// let arg_value = PossibleValue::new("fast").visible_alias("quick").alias("not-slow");
///
/// assert!(arg_value.matches("fast", false));
/// assert!(arg_value.matches("quick", false));
/// assert!(arg_value.matches("not-slow", false));
///
/// assert!(arg_value.matches("FAST", true));
Expand All @@ -223,6 +266,15 @@ impl PossibleValue {
}
}

/// Quote a string if it contains whitespace
fn quote_if_whitespace(n: &Str) -> std::borrow::Cow<'_, str> {
if n.contains(char::is_whitespace) {
format!("{:?}", n).into()
} else {
n.as_str().into()
}
}

impl<S: Into<Str>> From<S> for PossibleValue {
fn from(s: S) -> Self {
Self::new(s)
Expand Down
41 changes: 37 additions & 4 deletions src/output/help_template.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ impl<'cmd, 'writer> HelpTemplate<'cmd, 'writer> {
if let Some(arg) = arg {
const DASH_SPACE: usize = "- ".len();
const COLON_SPACE: usize = ": ".len();
const COMMA_SPACE: usize = ", ".len();
let possible_vals = arg.get_possible_values();
if self.use_long
&& !arg.is_hide_possible_values_set()
Expand All @@ -620,7 +621,19 @@ impl<'cmd, 'writer> HelpTemplate<'cmd, 'writer> {
.expect("Only called with possible value");
let help_longest = possible_vals
.iter()
.filter_map(|f| f.get_visible_help().map(|h| h.display_width()))
.map(|f| {
let help_width =
f.get_visible_help().map(|h| h.display_width()).unwrap_or(0);
let aliases = f.get_quoted_visible_aliases();
let alias_width: usize = aliases.iter().map(|a| display_width(a)).sum();
let comma_width = aliases.len().saturating_sub(1) * COMMA_SPACE;
let wrapper_width = if aliases.is_empty() {
0
} else {
" (alias: )".len()
};
help_width + alias_width + comma_width + wrapper_width
})
.max()
.expect("Only called with possible value with help");
// should new line
Expand All @@ -642,7 +655,19 @@ impl<'cmd, 'writer> HelpTemplate<'cmd, 'writer> {
self.spaces(spaces);
self.none("- ");
self.literal(pv.get_name());
if let Some(help) = pv.get_help() {
let mut opt_help = pv.get_help().cloned();
let aliases = pv.get_quoted_visible_aliases().join(", ");
if !aliases.is_empty() {
let mut help = opt_help.unwrap_or_default();
if !help.is_empty() {
help.none(" ");
}
help.none("(alias: ");
help.none(aliases);
help.none(")");
opt_help = Some(help);
}
if let Some(mut help) = opt_help {
debug!("HelpTemplate::help: Possible Value help");

if possible_value_new_line {
Expand All @@ -660,7 +685,6 @@ impl<'cmd, 'writer> HelpTemplate<'cmd, 'writer> {
usize::MAX
};

let mut help = help.clone();
replace_newline_var(&mut help);
help.wrap(avail_chars);
help.indent("", &trailing_indent);
Expand Down Expand Up @@ -783,7 +807,16 @@ impl<'cmd, 'writer> HelpTemplate<'cmd, 'writer> {

let pvs = possible_vals
.iter()
.filter_map(PossibleValue::get_visible_quoted_name)
.filter_map(|pv| {
pv.get_visible_quoted_name().map(|n| {
let aliases = pv.get_quoted_visible_aliases().join(", ");
if aliases.is_empty() {
n
} else {
format!("{n} (alias: {aliases})").into()
}
})
})
.collect::<Vec<_>>()
.join(", ");

Expand Down
3 changes: 2 additions & 1 deletion tests/derive/help.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ Arguments:
Argument help

Possible values:
- foo: Foo help
- foo: Foo help (alias: foozle)
epage marked this conversation as resolved.
Show resolved Hide resolved
- bar: Bar help

Options:
Expand All @@ -414,6 +414,7 @@ Options:
#[derive(clap::ValueEnum, PartialEq, Debug, Clone)]
enum ArgChoice {
/// Foo help
#[value(visible_alias("foozle"), alias("floofy"))]
Foo,
/// Bar help
Bar,
Expand Down