Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
16 changes: 16 additions & 0 deletions clap_complete/src/engine/candidate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ pub struct CompletionCandidate {
tag: Option<StyledStr>,
display_order: Option<usize>,
hidden: bool,
nospace: bool,
}

impl CompletionCandidate {
Expand Down Expand Up @@ -60,6 +61,16 @@ impl CompletionCandidate {
self
}

/// Suppress trailing space after this candidate
///
/// This is useful for completions like directory paths (ending in `/`),
/// `--flag=` values, and delimiter-separated values where a trailing space
/// would be incorrect.
Comment on lines +66 to +68
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is useful when there are more completion candidates if this completion is selected, like ...

pub fn nospace(mut self, nospace: bool) -> Self {
self.nospace = nospace;
self
}

/// Add a prefix to the value of completion candidate
///
/// This is generally used for post-process by [`complete`][crate::engine::complete()] for
Expand Down Expand Up @@ -104,6 +115,11 @@ impl CompletionCandidate {
pub fn is_hide_set(&self) -> bool {
self.hidden
}

/// Get whether trailing space should be suppressed
pub fn is_nospace_set(&self) -> bool {
self.nospace
}
}

impl<S: Into<OsString>> From<S> for CompletionCandidate {
Expand Down
31 changes: 24 additions & 7 deletions clap_complete/src/engine/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ fn complete_option(
completions.extend(
complete_arg_value(value.to_str().ok_or(value), arg, current_dir)
.into_iter()
.map(|comp| comp.add_prefix(format!("--{flag}="))),
.map(|comp| comp.add_prefix(format!("--{flag}=")).nospace(true)),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this unconditionally adding nospace?

);
}
} else {
Expand Down Expand Up @@ -308,7 +308,12 @@ fn complete_option(
.into_iter()
.map(|comp| {
let sep = if has_equal { "=" } else { "" };
comp.add_prefix(format!("-{leading_flags}{sep}"))
let comp = comp.add_prefix(format!("-{leading_flags}{sep}"));
if has_equal {
comp.nospace(true)
} else {
comp
}
Comment on lines +311 to +316
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why?

}),
);
} else {
Expand Down Expand Up @@ -393,7 +398,7 @@ fn complete_arg_value(
if let Some(prefix) = prefix {
values = values
.into_iter()
.map(|comp| comp.add_prefix(prefix))
.map(|comp| comp.add_prefix(prefix).nospace(true))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why?

.collect();
}
values = values
Expand Down Expand Up @@ -478,9 +483,14 @@ fn longs_and_visible_aliases(p: &clap::Command) -> Vec<CompletionCandidate> {
p.get_arguments()
.filter_map(|a| {
a.get_long_and_visible_aliases().map(|longs| {
longs
.into_iter()
.map(|s| populate_arg_candidate(CompletionCandidate::new(format!("--{s}")), a))
longs.into_iter().map(|s| {
if a.is_require_equals_set() {
populate_arg_candidate(CompletionCandidate::new(format!("--{s}=")), a)
.nospace(true)
} else {
populate_arg_candidate(CompletionCandidate::new(format!("--{s}")), a)
Comment on lines +487 to +491
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't this conflict with your requires_equals PR?

I also think I prefer the approach taken in that PR.

}
})
})
})
.flatten()
Expand All @@ -495,7 +505,14 @@ fn hidden_longs_aliases(p: &clap::Command) -> Vec<CompletionCandidate> {
.filter_map(|a| {
a.get_aliases().map(|longs| {
longs.into_iter().map(|s| {
populate_arg_candidate(CompletionCandidate::new(format!("--{s}")), a).hide(true)
if a.is_require_equals_set() {
populate_arg_candidate(CompletionCandidate::new(format!("--{s}=")), a)
.hide(true)
.nospace(true)
} else {
populate_arg_candidate(CompletionCandidate::new(format!("--{s}")), a)
.hide(true)
}
})
})
})
Expand Down
3 changes: 2 additions & 1 deletion clap_complete/src/engine/custom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,8 @@ pub(crate) fn complete_path(
let mut suggestion = prefix.join(&raw_file_name);
suggestion.push(""); // Ensure trailing `/`
let candidate = CompletionCandidate::new(suggestion.as_os_str().to_owned())
.hide(is_hidden(&raw_file_name));
.hide(is_hidden(&raw_file_name))
.nospace(true);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we limit this to !is_wanted? I'm guessing not because something may look like a valid candidate but the user may instead want a directory inside of the current one.

Granted, besides sort order, we don't have a good way of communicating which directories match is_wanted and which don't. Maybe the way to deal with that is to provide a way to add a description for wanted paths.


if is_wanted(&entry.path()) {
completions.push(candidate);
Expand Down
163 changes: 163 additions & 0 deletions clap_complete/tests/testsuite/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1478,3 +1478,166 @@ fn complete(cmd: &mut Command, args: impl AsRef<str>, current_dir: Option<&Path>
.collect::<Vec<_>>()
.join("\n")
}

/// Like `complete` but includes `[nospace]` marker for candidates with nospace set
fn complete_with_nospace(
Comment on lines +1482 to +1483
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We generally ask for tests to be added in a prior commit, with them passing in that commit

cmd: &mut Command,
args: impl AsRef<str>,
current_dir: Option<&Path>,
) -> String {
let input = args.as_ref();
let mut args = vec![std::ffi::OsString::from(cmd.get_name())];
let arg_index;

if let Some((prior, after)) = input.split_once("[TAB]") {
args.extend(prior.split_whitespace().map(From::from));
if prior.ends_with(char::is_whitespace) {
args.push(std::ffi::OsString::default());
}
arg_index = args.len() - 1;
args.extend(after.split_whitespace().map(From::from));
} else {
args.extend(input.split_whitespace().map(From::from));
if input.ends_with(char::is_whitespace) {
args.push(std::ffi::OsString::default());
}
arg_index = args.len() - 1;
}

clap_complete::engine::complete(cmd, args, arg_index, current_dir)
.unwrap()
.into_iter()
.map(|candidate| {
let compl = candidate.get_value().to_str().unwrap();
let nospace = if candidate.is_nospace_set() {
"[nospace]"
} else {
""
};
if let Some(help) = candidate.get_help() {
format!("{compl}\t{help}{nospace}")
} else if !nospace.is_empty() {
format!("{compl}\t{nospace}")
} else {
compl.to_owned()
}
})
.collect::<Vec<_>>()
.join("\n")
}

#[test]
fn nospace_on_directory_completions() {
let testdir = snapbox::dir::DirRoot::mutable_temp().unwrap();
let testdir_path = testdir.path().unwrap();

fs::write(testdir_path.join("a_file"), "").unwrap();
fs::create_dir_all(testdir_path.join("b_dir")).unwrap();

let mut cmd = Command::new("dynamic").arg(
clap::Arg::new("input")
.long("input")
.value_hint(clap::ValueHint::AnyPath),
);

assert_data_eq!(
complete_with_nospace(&mut cmd, "--input [TAB]", Some(testdir_path)),
snapbox::str![[r#"
.
a_file
b_dir/ [nospace]
"#]],
);
}

#[test]
fn nospace_on_equals_value_completions() {
let mut cmd = Command::new("dynamic").arg(
clap::Arg::new("format")
.long("format")
.value_parser(["json", "yaml"]),
);

assert_data_eq!(
complete_with_nospace(&mut cmd, "--format=[TAB]", None),
snapbox::str![[r#"
--format=json [nospace]
--format=yaml [nospace]
"#]],
);
}

#[test]
fn nospace_on_short_equals_value_completions() {
let mut cmd = Command::new("dynamic").arg(
clap::Arg::new("format")
.long("format")
.short('F')
.value_parser(["json", "yaml"]),
);

assert_data_eq!(
complete_with_nospace(&mut cmd, "-F=[TAB]", None),
snapbox::str![[r#"
-F=json [nospace]
-F=yaml [nospace]
"#]],
);

// Without equals, no nospace
assert_data_eq!(
complete_with_nospace(&mut cmd, "-F [TAB]", None),
snapbox::str![[r#"
json
yaml
"#]],
);
}

#[test]
fn nospace_on_require_equals_options() {
let mut cmd = Command::new("dynamic").arg(
clap::Arg::new("format")
.long("format")
.require_equals(true)
.value_parser(["json", "yaml"]),
);

assert_data_eq!(
complete_with_nospace(&mut cmd, "--[TAB]", None),
snapbox::str![[r#"
--format= [nospace]
--help Print help
"#]],
);
}

#[test]
fn nospace_on_delimiter_completions() {
let mut cmd = Command::new("delimiter").arg(
clap::Arg::new("delimiter")
.long("delimiter")
.value_parser(["comma", "space", "tab"])
.value_delimiter(','),
);

// First value: no nospace (no prefix)
assert_data_eq!(
complete_with_nospace(&mut cmd, "--delimiter [TAB]", None),
snapbox::str![[r#"
comma
space
tab
"#]],
);

// After delimiter: nospace because of prefix
assert_data_eq!(
complete_with_nospace(&mut cmd, "--delimiter comma,[TAB]", None),
snapbox::str![[r#"
comma,comma [nospace]
comma,space [nospace]
comma,tab [nospace]
"#]],
);
}