Skip to content

refactor(completion): move NuMatcher from nu-cli to nu-protocol#17757

Draft
blindFS wants to merge 5 commits intonushell:mainfrom
blindFS:completion
Draft

refactor(completion): move NuMatcher from nu-cli to nu-protocol#17757
blindFS wants to merge 5 commits intonushell:mainfrom
blindFS:completion

Conversation

@blindFS
Copy link
Copy Markdown
Contributor

@blindFS blindFS commented Mar 9, 2026

This PR serves as the preparation work for some follow-up PRs that implement a parameter-level dynamic completer.

As discussed in #16859, something like:

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
pub enum Completion {
    Command(DeclId),
    List(NuCow<&'static [&'static str], Vec<String>>),
    Builtin(Box<dyn ArgumentCompleter>), // <------- new
}

will make the life a lot easier.

I believe it's actually feasible with the trick of #[typetag::serde(tag = "type")] and dyn-clone.
So there’s going to be quite some refactoring. To make it easier to review, I think I'll try to break it into small PRs.

In order to make parameter-level completers only return results that match the user input, IMO NuMatcher is best suited in nu-protocol/src/completion/matcher.rs, hence this PR. @ysthakur

Release notes summary - What our users need to know

Nothing

Tasks after submitting

  1. Make DotNuCompletion, ExportableCompletion, CommandCompletion ... implement the trait ArgumentCompleter and move them to nu-protocol/src/completion
  2. Bind them to the parameters of command use/export use/which/attr complete/... right in the signature definition
  3. Clean-up get_dynamic_completion related stuff introduced in Plugin: support custom completions in command flags #16859 and fix(completion): path/directory/glob typed flag values get empty completion results #17006
  4. (Optional) make those builtin completers available to user-defined nushell commands. Through new syntax or specific prefixed completer names like foo: string@"builtin exportable"

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 9, 2026

Hey, just a bot checking in! You edited files related to the configuration.
If you changed any of the default values or added a new config option, don't forget to update the doc_config.nu which documents the options for our users including the defaults provided by the Rust implementation.
If you didn't make a change here, you can just ignore me.

@blindFS blindFS marked this pull request as ready for review March 10, 2026 01:18
Copy link
Copy Markdown
Member

@ysthakur ysthakur left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread crates/nu-cli/src/completions/matcher_helper.rs Outdated
@ysthakur
Copy link
Copy Markdown
Member

I'm wondering if we should go so far as to move NuMatcher into Reedline. After all, it's not really specific to Nushell

@ysthakur
Copy link
Copy Markdown
Member

Please take care of the failed security audit check, looks like this uses dependencies that need to be updated/replaced? https://github.com/nushell/nushell/actions/runs/22882189266/job/66387247251?pr=17757

Co-authored-by: Yash Thakur <45539777+ysthakur@users.noreply.github.com>
@blindFS
Copy link
Copy Markdown
Contributor Author

blindFS commented Mar 10, 2026

Please take care of the failed security audit check, looks like this uses dependencies that need to be updated/replaced? https://github.com/nushell/nushell/actions/runs/22882189266/job/66387247251?pr=17757

Seems addressed by #17765

@blindFS
Copy link
Copy Markdown
Contributor Author

blindFS commented Mar 10, 2026

I'm wondering if we should go so far as to move NuMatcher into Reedline. After all, it's not really specific to Nushell

As I commented here, my ideal way of handling matching while typing would require NuMatcher on both sides. But if the candidates aren't too many (or we hard limit the amount), pre-filtering could be saved?

@blindFS blindFS marked this pull request as draft March 11, 2026 09:00
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