feat(complete): Add per-candidate trailing space control#6266
feat(complete): Add per-candidate trailing space control#6266AndreasBackx wants to merge 2 commits intoclap-rs:masterfrom
Conversation
…ompletions Add a `nospace: bool` field to `CompletionCandidate` that tells shell adapters to suppress the trailing space after a completion. This is set on: - Directory path completions (ending in `/`) - `--flag=value` inline completions (long and short) - Delimiter-prefixed completions (e.g., `comma,space`) - `require_equals` options (completed with trailing `=`) The field defaults to `false` and is exposed via: - Builder: `pub fn nospace(mut self, nospace: bool) -> Self` - Getter: `pub fn is_nospace_set(&self) -> bool` This is the first part of clap-rs#5587; per-shell handling follows.
Each shell adapter now translates the `nospace` field into its native mechanism: - **Bash**: Completions output a `_CLAP_COMPLETE_NOSPACE` sentinel as the last entry when any candidate has nospace set. The registration script detects this sentinel, strips it, and calls `compopt -o nospace`. This replaces the previous `[=/:]$` regex heuristic. - **Zsh**: Nospace candidates are prefixed with `\x1f` (unit separator) in the wire output. The registration script splits candidates into `nospace` and `other` groups, using `_describe 'values' nospace -S ''` for the former. This replaces the previous directory-only split. - **Elvish**: Nospace candidates are prefixed with `\x1f`. The registration script pipes output through `from-lines` and wraps nospace values in `edit:complex-candidate $value &code-suffix=''`. - **Fish**: No changes needed — Fish naturally suppresses trailing space for `/`-terminated completions and handles `=`-joined tokens through its tokenization. - **PowerShell**: No changes — `CompletionResult` does not support per-candidate nospace control. Closes clap-rs#5587
| /// Like `complete` but includes `[nospace]` marker for candidates with nospace set | ||
| fn complete_with_nospace( |
There was a problem hiding this comment.
We generally ask for tests to be added in a prior commit, with them passing in that commit
| /// This is useful for completions like directory paths (ending in `/`), | ||
| /// `--flag=` values, and delimiter-separated values where a trailing space | ||
| /// would be incorrect. |
There was a problem hiding this comment.
This is useful when there are more completion candidates if this completion is selected, like ...
| 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)), |
There was a problem hiding this comment.
Why is this unconditionally adding nospace?
| let comp = comp.add_prefix(format!("-{leading_flags}{sep}")); | ||
| if has_equal { | ||
| comp.nospace(true) | ||
| } else { | ||
| comp | ||
| } |
| values = values | ||
| .into_iter() | ||
| .map(|comp| comp.add_prefix(prefix)) | ||
| .map(|comp| comp.add_prefix(prefix).nospace(true)) |
| 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) |
There was a problem hiding this comment.
Doesn't this conflict with your requires_equals PR?
I also think I prefer the approach taken in that PR.
| let candidate = CompletionCandidate::new(suggestion.as_os_str().to_owned()) | ||
| .hide(is_hidden(&raw_file_name)); | ||
| .hide(is_hidden(&raw_file_name)) | ||
| .nospace(true); |
There was a problem hiding this comment.
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.
| let has_nospace = completions.iter().any(|c| c.is_nospace_set()); | ||
|
|
||
| for (i, candidate) in completions.iter().enumerate() { | ||
| if i != 0 { | ||
| write!(buf, "{}", ifs.as_deref().unwrap_or("\n"))?; | ||
| } | ||
| write!(buf, "{}", candidate.get_value().to_string_lossy())?; | ||
| } | ||
| if has_nospace && !completions.is_empty() { | ||
| write!(buf, "{}", ifs.as_deref().unwrap_or("\n"))?; | ||
| write!(buf, "_CLAP_COMPLETE_NOSPACE")?; | ||
| } |
There was a problem hiding this comment.
This makes it so if any candidate doesn't have a space, none of them do.
Instead, if nospace is supported, could we always append a space and have bash always add nospace?
| local -a nospace=() | ||
| local -a other=() | ||
| local completion | ||
| for completion in $completions; do | ||
| local value="${completion%%:*}" | ||
| if [[ "$value" == */ ]]; then | ||
| local dir_no_slash="${value%/}" | ||
| if [[ "$completion" == *:* ]]; then | ||
| local desc="${completion#*:}" | ||
| dirs+=("$dir_no_slash:$desc") | ||
| else | ||
| dirs+=("$dir_no_slash") | ||
| fi | ||
| if [[ "$completion" == $'\x1f'* ]]; then | ||
| completion="${completion:1}" | ||
| nospace+=("$completion") | ||
| else | ||
| other+=("$completion") | ||
| fi | ||
| done | ||
| [[ -n $dirs ]] && _describe 'values' dirs -S '/' -r '/' | ||
| [[ -n $nospace ]] && _describe 'values' nospace -S '' |
There was a problem hiding this comment.
Again, this is doing it all or nothing
Summary
Adds a
nospacefield toCompletionCandidateso the engine can tell shells "don't add a trailing space after this completion." Each shell adapter translates this into its native mechanism, replacing ad-hoc heuristics.Fixes #5587
Changes
clap_complete/src/engine/candidate.rs: Addnospace: boolfield with builder.nospace(bool)and getter.is_nospace_set().clap_complete/src/engine/custom.rs: Setnospace(true)on directory candidates incomplete_path().clap_complete/src/engine/complete.rs: Setnospace(true)on--flag=valuecompletions,-F=valuecompletions, delimiter-prefixed completions, andrequire_equalsoptions with=suffix.clap_complete/src/env/shells.rs:_CLAP_COMPLETE_NOSPACEsentinel; registration script detects it and callscompopt -o nospace. Replaces the[=/:]$regex heuristic.\x1f; registration script splits intonospace/othergroups using_describe 'values' nospace -S ''. Replaces directory-only split.\x1f; registration script wraps them inedit:complex-candidate $value &code-suffix=''.clap_complete/tests/testsuite/engine.rs: 5 new tests for nospace on directories, equals values, short equals,require_equals, and delimiters.Test Plan
cargo test -p clap_complete --features unstable-dynamic— 112 tests passcargo clippy -p clap_complete --features unstable-dynamic— no warningscargo fmt -p clap_complete --check— clean