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.

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

WIP: fix(complete): escaping in zsh completion #3713

Closed
wants to merge 1 commit into from

Conversation

pseyfert
Copy link
Contributor

@pseyfert pseyfert commented May 9, 2022

I basically went through the reproducer from #1596 and tried to narrow down which characters cause issues in:

fn main() {                                                                                                                 
    let values = (0u8..128u8)                             
        .map(|i| i as char)                                                                                           
        .filter(|c| !c.is_ascii_control())                                                                            
        .map(|c| c.to_string())                                                                                       
        .collect::<Vec<_>>();                                                                                        
                                                                                                                      
    let mut app = Command::new("clap-zsh-completions-repro");                                                         
                       
    app = app.arg(Arg::new("kuku").required(true).possible_values(values.iter().map(|v| PossibleValue::new(v).help("crazy char")).collect::<Vec<PossibleValue>>())); // without tooltip
    // app = app.arg(Arg::new("kuku").required(true).possible_values(values.iter().map(|v| PossibleValue::new(v)).collect::<Vec<PossibleValue>>())); // with tooltip
    
    app.set_bin_name("my_app");                                                                                       
    generate(Zsh, &mut app, "my_app", &mut io::stdout());                                                             
}   

turns out, there are a few edge cases (due to the tool tip handling) that need different escaping in the two versions. In a few cases (see comment) escaping is only needed in without-tooltip, but doesn't lead to wrong suggestions with-tooltip, so i didn't add a distinction. Note: i only went through what comes in with .possible_values. (at time of writing). I didn't check what happens to characters in PossibleValue::help or other strings that get written to completion functions

from the top of my head, that could be

  • Arg(...).value_name[s]
  • Arg(...).help
  • Arg(...).long

I'm a tad surprised that the escaping for single quote that was already in the code base didn't work for me …

Marking as WIP because I'm wondering: should we pursue this further? is fancy characters just a won't-fix? do we want to cover the other user-defined strings exhaustively before merging?

 * This escapes everything that comes up in the reproducer from the
   above issue. NB: this means possible_values only.

missing:

 * possible_value(...).help
 * Arg(...).value_name[s]
 * Arg(...).about
 * Arg(...).long
@epage
Copy link
Member

epage commented May 10, 2022

Marking as WIP because I'm wondering: should we pursue this further? is fancy characters just a won't-fix? do we want to cover the other user-defined strings exhaustively before merging?

I'd be more comfortable with this if we could say why certain characters are allowed in some positions but not others and made the design centered on that, rather than on trial and error. It'd help make the code clearer if nothing else.

@epage
Copy link
Member

epage commented Jan 3, 2023

Without updates from the author, I'm going ahead and closing this out.

@epage epage closed this Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants