Skip to content

Commit b798f1f

Browse files
authored
Merge pull request #382 from pacak/complete3
Completion: move careful handling of mix of positional and non-positional items
2 parents d10410f + 94c13f4 commit b798f1f

File tree

6 files changed

+365
-36
lines changed

6 files changed

+365
-36
lines changed

src/args.rs

+5
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,11 @@ impl Args<'_> {
5555
/// .unwrap_stdout();
5656
/// assert_eq!(r, "-f");
5757
/// ```
58+
///
59+
/// Note to self: shell passes "" as a parameter in situations like foo `--bar TAB`, bpaf
60+
/// completion stubs adopt this conventions add pass it along. This is needed so completer can
61+
/// tell the difference between `--bar` being completed or an argument to it in the example
62+
/// above.
5863
#[cfg(feature = "autocomplete")]
5964
#[must_use]
6065
pub fn set_comp(mut self, rev: usize) -> Self {

src/complete_gen.rs

+23-12
Original file line numberDiff line numberDiff line change
@@ -166,14 +166,15 @@ impl State {
166166
}
167167

168168
impl Complete {
169-
pub(crate) fn push_shell(&mut self, op: ShellComp, depth: usize) {
169+
pub(crate) fn push_shell(&mut self, op: ShellComp, is_argument: bool, depth: usize) {
170170
self.comps.push(Comp::Shell {
171171
extra: CompExtra {
172172
depth,
173173
group: None,
174174
help: None,
175175
},
176176
script: op,
177+
is_argument,
177178
});
178179
}
179180

@@ -224,10 +225,7 @@ pub(crate) struct CompExtra {
224225
#[derive(Clone, Debug)]
225226
pub(crate) enum Comp {
226227
/// short or long flag
227-
Flag {
228-
extra: CompExtra,
229-
name: ShortLong,
230-
},
228+
Flag { extra: CompExtra, name: ShortLong },
231229

232230
/// argument + metadata
233231
Argument {
@@ -256,12 +254,15 @@ pub(crate) enum Comp {
256254
Metavariable {
257255
extra: CompExtra,
258256
meta: &'static str,
257+
/// AKA not positional
259258
is_argument: bool,
260259
},
261260

262261
Shell {
263262
extra: CompExtra,
264263
script: ShellComp,
264+
/// AKA not positional
265+
is_argument: bool,
265266
},
266267
}
267268

@@ -348,6 +349,9 @@ fn pair_to_os_string<'a>(pair: (&'a Arg, &'a OsStr)) -> Option<(&'a Arg, &'a str
348349
Some((pair.0, pair.1.to_str()?))
349350
}
350351

352+
/// What is the preceeding item, if any
353+
///
354+
/// Mostly is there to tell if we are trying to complete and argument or not...
351355
#[derive(Debug, Copy, Clone)]
352356
enum Prefix<'a> {
353357
NA,
@@ -374,7 +378,7 @@ impl State {
374378
// try get a current item to complete - must be non-virtual right most one
375379
// value must be present here, and can fail only for non-utf8 values
376380
// can't do much completing with non-utf8 values since bpaf needs to print them to stdout
377-
let (_, lit) = items.next()?;
381+
let (cur, lit) = items.next()?;
378382

379383
// For cases like "-k=val", "-kval", "--key=val", "--key val"
380384
// last value is going to be either Arg::Word or Arg::ArgWord
@@ -389,13 +393,18 @@ impl State {
389393
_ => (false, lit),
390394
};
391395

396+
let is_named = match cur {
397+
Arg::Short(_, _, _) | Arg::Long(_, _, _) => true,
398+
Arg::ArgWord(_) | Arg::Word(_) | Arg::PosWord(_) => false,
399+
};
400+
392401
let prefix = match preceeding {
393402
Some((Arg::Short(s, true, _os), _lit)) => Prefix::Short(*s),
394403
Some((Arg::Long(l, true, _os), _lit)) => Prefix::Long(l.as_str()),
395404
_ => Prefix::NA,
396405
};
397406

398-
let (items, shell) = comp.complete(lit, pos_only, prefix);
407+
let (items, shell) = comp.complete(lit, pos_only, is_named, prefix);
399408

400409
Some(match comp.output_rev {
401410
0 => render_test(&items, &shell, full_lit),
@@ -480,10 +489,9 @@ impl Comp {
480489
fn only_value(&self) -> bool {
481490
match self {
482491
Comp::Flag { .. } | Comp::Argument { .. } | Comp::Command { .. } => false,
483-
Comp::Metavariable { is_argument, .. } | Comp::Value { is_argument, .. } => {
484-
*is_argument
485-
}
486-
Comp::Shell { .. } => true,
492+
Comp::Metavariable { is_argument, .. }
493+
| Comp::Value { is_argument, .. }
494+
| Comp::Shell { is_argument, .. } => *is_argument,
487495
}
488496
}
489497
fn is_pos(&self) -> bool {
@@ -500,6 +508,7 @@ impl Complete {
500508
&self,
501509
arg: &str,
502510
pos_only: bool,
511+
is_named: bool,
503512
prefix: Prefix,
504513
) -> (Vec<ShowComp>, Vec<ShellComp>) {
505514
let mut items: Vec<ShowComp> = Vec::new();
@@ -588,7 +597,9 @@ impl Complete {
588597
}
589598

590599
Comp::Shell { script, .. } => {
591-
shell.push(*script);
600+
if !is_named {
601+
shell.push(*script);
602+
}
592603
}
593604
}
594605
}

src/complete_shell.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,8 @@ where
9797
let depth = args.depth();
9898
if let Some(comp) = args.comp_mut() {
9999
for ci in comp_items {
100-
if ci.is_metavar().is_some() {
101-
comp.push_shell(self.op, depth);
100+
if let Some(is_argument) = ci.is_metavar() {
101+
comp.push_shell(self.op, is_argument, depth);
102102
} else {
103103
comp.push_comp(ci);
104104
}

src/structs.rs

+47-22
Original file line numberDiff line numberDiff line change
@@ -368,29 +368,54 @@ fn this_or_that_picks_first(
368368
};
369369

370370
#[cfg(feature = "autocomplete")]
371-
let len_a = args_a.len();
372-
373-
#[cfg(feature = "autocomplete")]
374-
let len_b = args_b.len();
375-
376-
#[cfg(feature = "autocomplete")]
377-
if let (Some(a), Some(b)) = (args_a.comp_mut(), args_b.comp_mut()) {
378-
// if both parsers managed to consume the same amount - including 0, keep
379-
// results from both, otherwise keep results from one that consumed more
380-
let (keep_a, keep_b) = match res {
381-
Ok((true, _)) => (true, false),
382-
Ok((false, _)) => (false, true),
383-
Err(_) => match len_a.cmp(&len_b) {
384-
std::cmp::Ordering::Less => (true, false),
385-
std::cmp::Ordering::Equal => (true, true),
386-
std::cmp::Ordering::Greater => (false, true),
387-
},
388-
};
389-
if keep_a {
390-
comp_stash.extend(a.drain_comps());
371+
{
372+
let mut keep_a = true;
373+
let mut keep_b = true;
374+
if args_a.len() != args_b.len() {
375+
// If neither parser consumed anything - both can produce valid completions, otherwise
376+
// look for the first "significant" consume and keep that parser
377+
//
378+
// This is needed to preserve completion from a choice between a positional and a flag
379+
// See https://github.com/pacak/bpaf/issues/303 for more details
380+
if let (Some(_), Some(_)) = (args_a.comp_mut(), args_b.comp_mut()) {
381+
'check: for (ix, arg) in args_a.items.iter().enumerate() {
382+
// During completion process named and unnamed arguments behave
383+
// different - `-` and `--` are positional arguments, but we want to produce
384+
// named items too. An empty string is also a special type of item that
385+
// gets passed when user starts completion without passing any actual data.
386+
//
387+
// All other strings are either valid named items or valid positional items
388+
// those are hopefully follow the right logic for being parsed/not parsed
389+
if ix + 1 == args_a.items.len() {
390+
let os = arg.os_str();
391+
if os.is_empty() || os == "-" || os == "--" {
392+
break 'check;
393+
}
394+
}
395+
if let (Some(a), Some(b)) = (args_a.present(ix), args_b.present(ix)) {
396+
match (a, b) {
397+
(false, true) => {
398+
keep_b = false;
399+
break 'check;
400+
}
401+
(true, false) => {
402+
keep_a = false;
403+
break 'check;
404+
}
405+
_ => {}
406+
}
407+
}
408+
}
409+
}
391410
}
392-
if keep_b {
393-
comp_stash.extend(b.drain_comps());
411+
412+
if let (Some(a), Some(b)) = (args_a.comp_mut(), args_b.comp_mut()) {
413+
if keep_a {
414+
comp_stash.extend(a.drain_comps());
415+
}
416+
if keep_b {
417+
comp_stash.extend(b.drain_comps());
418+
}
394419
}
395420
}
396421

0 commit comments

Comments
 (0)