-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(complete): Add dynamic completion for nushell #5841
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
base: master
Are you sure you want to change the base?
Conversation
|
I do have some questions:
|
For dynamic completions, we focus on the shell integration
etc e.g. see clap/clap_complete/tests/testsuite/bash.rs Lines 242 to 338 in fde45f9
Note that I'm going to focus first on the issue to see if there is any points there ti work out before coming back to the PR to review the implementation. |
|
re:tests: Thanks! I'll have a look at these. Getting them up and running for nushell should be relatively independent of the concrete implementation.
Yep, let's nail down the conceptual aspects first. |
23fc096 to
f58c6ee
Compare
|
I have updated the implementation to account for changes in nushell since the beginning of the year ( Tests are still a TODO. |
bc9b84f to
3ef74cb
Compare
|
Another update: # user runs (example: jj)
COMPLETE=nushell jj | save --append --raw $nu.env-path# env.nu (example: jj)
# Refresh completer integration for jj (must be in env.nu)
do {
# Search for existing script to avoid duplicates in case autoload dirs change
let completer_script_name = 'jj-completer.nu'
let autoload_dir = $nu.user-autoload-dirs
| where { path join $completer_script_name | path exists }
| get 0 --optional
| default ($nu.user-autoload-dirs | get 0 --optional)
mkdir $autoload_dir
let completer_path = ($autoload_dir | path join $completer_script_name)
COMPLETE=nushell _COMPLETE__mode=integration ^r#'/home/USER/.cargo/bin/jj'# | save --raw --force $completer_path
}and this will maintain # Performs the completion for jj
def jj-completer [
spans: list<string> # The spans that were passed to the external completer closure
]: nothing -> list {
COMPLETE=nushell ^r#'/home/chris/.cargo/bin/jj'# -- ...$spans | from json
}
@complete jj-completer
def --wrapped jj [...args] {
^r#'/home/chris/.cargo/bin/jj'# ...$args
} |
clap_complete_nushell/src/dynamic.rs
Outdated
| } | ||
|
|
||
| fn is(&self, name: &str) -> bool { | ||
| name.eq_ignore_ascii_case("nushell") || name.eq_ignore_ascii_case("nu") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this is case ignoring and accepting both names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary motivation was the robustness principle (and I'm well aware of the downsides/criticism of the principle). As a user of nu/nushell, I get the impression that they cannot make up their mind as to what the official name for their shell is. The binary and the repository are called nu, the website, social media accounts, etc. are all called nushell
So the question is: what do we want people to type COMPLETE=nu their-clap-binary (to match the name of the binary/command used to launch nushell) or do we want people to type COMPLETE=nushell their-clap-binary (to match the marketing name for nushell)?
I personally tend towards nushell, but I don't have a very strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary motivation was the robustness principle (and I'm well aware of the downsides/criticism of the principle).
In terms of case insensitivity, that is best left to a wider conversation on how we should handle this for all shells while this PR conforms to our existing practices.
As for names, so far we only accept the file stem stored in SHELL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that the powershell integration also uses powershell (instead of the binary name pwsh) => I'll change it to just match nushell (case sensitive)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is again related to the SHELL lookup (#4447). If that is incorrect, then that is a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To re-iterate differently, since SHELL would end up pointing to nu, that is the name we should use
clap_complete_nushell/src/dynamic.rs
Outdated
| buf: &mut dyn Write, | ||
| ) -> Result<(), Error> { | ||
| let mode_var = format!("{}", ModeVar(var)); | ||
| if std::env::var_os(&mode_var).as_ref().map(|x| x.as_os_str()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating an internal environment variable in what is needing to be a public interface is something we'll need to consider more thoroughly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One alternative would be to re-use the COMPLETE variable. We could register two "shells" (or maybe dispatch based on the name used?)
COMPLETE=nushell your-clap-tool generates the auto-update code; COMPLETE=nushell-registration your-clap-tool generates the actual integration code.
Seems a bit more of a hack on the one hand, but it would be safer in the sense that the two are mutually exclusive and in that we don't use some other, undocumented variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I want to switch all completions to a two-step process for #5668 and have been wondering about doing something like that. This is all unstable so we can change it over time so we don't need to block on it but we will eventually need to figure it out.
Adds an implementation of dynamic completion to `clap_complete_nushell` under the `unstable-dynamic` feature flag. The user runs ```nushell COMPLETE=nushell the-clap-tool | save --append --raw $nu.env-path ``` and the dynamic completion will emit an "auto-update" script into the user's `env.nu`. The auto-update script snippet will maintain a script in the user's autoload directory that installs a command-scoped completer.
3ef74cb to
634e750
Compare
| // Std | ||
| use std::ffi::{OsStr, OsString}; | ||
| use std::fmt::Display; | ||
| use std::io::{Error, Write}; | ||
| use std::path::Path; | ||
|
|
||
| // External | ||
| use clap::Command; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't comment the groupings
| use clap_complete::Generator; | ||
|
|
||
| /// Generate Nushell complete file | ||
| #[derive(Copy, Clone, PartialEq, Eq, Debug)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still needed?
| use clap_complete::env::EnvCompleter; | ||
|
|
||
| /// Generate integration for dynamic completion in Nushell | ||
| #[derive(Copy, Clone, PartialEq, Eq, Debug)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you have all of these traits implemented?
Adds an implementation of dynamic completion to
clap_complete_nushellunder theunstable-dynamicfeature flag. Corresponding issue #5840. The issue description has more context and a more complete solution sketch. This PR doesn't implement the full solution sketched there. Ideally, we'd first agree that that solution is the way to go.With this PR (and the
unstable-dynamicfeature flag), clap tools can generate a nushell module that offers three commands:complete, which performs the actual completionhandles, which asks the module whether it is the correct completer for a particular command lineinstall, which globally registers a completer that falls back to whatever completer was previously installed ifhandlesrejects completing a command line.The idea is that user's who already have a custom external completer can integrate the generated module's
handlesandcompletecommands into their completer.Everyone else just puts
into their nushell config.
To test it, I have integrated it into a local build of the relatively complex clap-based tool, JJ (Jujutsu), and it worked very well so far.
TODO