fix: Respect sort in custom completions (#14424)

<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description

This PR makes it so that when a custom completer sets `options.sort` to
true, completions aren't sorted. Previously, in #13311, I'd made it so
that setting `sort` to true would sort in alphabetical order, while
omitting it or setting it to false would sort it in the default order
for the chosen match algorithm (alphabetical for prefix matching, fuzzy
match score for fuzzy matching). I'd assumed that you'd always want to
sort completions and the important thing was choosing alphabetical
sorting vs the default sort order for your match algorithm. However,
this assumption was incorrect (see #13696 and [this
thread](https://discord.com/channels/601130461678272522/1302332259227144294)
in Discord).

An alternative would be to make `sort` accept `"alphabetical"`,
`"smart"`, and `"none"`/`null` rather than keeping it a boolean. But
that would be a breaking change and require more discussion, and I
wanted to keep this PR simple/small so that we can go back to the
sensible behavior as soon as possible.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

Here are the different scenarios:
- If your custom completer returns a record with an `options` field
that's a record:
- If `options` contains `sort: true`, completions **will be sorted
according to the order set in the user's config**. Previously, they
would have been sorted in alphabetical order. This does mean that
**custom completers cannot explicitly choose to sort in alphabetical
order** anymore. I think that's an acceptable trade-off, though.
- If `options` contains `sort: false`, completions will not be sorted.
#13311 broke things so they would be sorted in the default order for the
match algorithm used. Before that PR, completions would not have been
sorted.
- If there's no `sort` option, that **will be treated as `sort: true`**.
Previously, this would have been treated as `sort: false`.
- Otherwise, nothing changes. Completions will still be sorted.

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the
tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

Added 1 test to make sure that completions aren't sorted with `sort:
false` explicitly set.

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
This commit is contained in:
Yash Thakur 2024-11-29 20:47:57 -05:00 committed by GitHub
parent 0172ad8461
commit 07a37f9b47
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 48 additions and 17 deletions

View file

@ -1,13 +1,12 @@
use crate::completions::{
completer::map_value_completions, Completer, CompletionOptions, MatchAlgorithm,
SemanticSuggestion,
completer::map_value_completions, Completer, CompletionOptions, SemanticSuggestion,
};
use nu_engine::eval_call;
use nu_protocol::{
ast::{Argument, Call, Expr, Expression},
debugger::WithoutDebug,
engine::{Stack, StateWorkingSet},
CompletionSort, DeclId, PipelineData, Span, Type, Value,
DeclId, PipelineData, Span, Type, Value,
};
use std::collections::HashMap;
@ -68,6 +67,7 @@ impl Completer for CustomCompletion {
);
let mut custom_completion_options = None;
let mut should_sort = true;
// Parse result
let suggestions = result
@ -85,10 +85,9 @@ impl Completer for CustomCompletion {
let options = val.get("options");
if let Some(Value::Record { val: options, .. }) = &options {
let should_sort = options
.get("sort")
.and_then(|val| val.as_bool().ok())
.unwrap_or(false);
if let Some(sort) = options.get("sort").and_then(|val| val.as_bool().ok()) {
should_sort = sort;
}
custom_completion_options = Some(CompletionOptions {
case_sensitive: options
@ -98,20 +97,16 @@ impl Completer for CustomCompletion {
positional: options
.get("positional")
.and_then(|val| val.as_bool().ok())
.unwrap_or(true),
.unwrap_or(completion_options.positional),
match_algorithm: match options.get("completion_algorithm") {
Some(option) => option
.coerce_string()
.ok()
.and_then(|option| option.try_into().ok())
.unwrap_or(MatchAlgorithm::Prefix),
.unwrap_or(completion_options.match_algorithm),
None => completion_options.match_algorithm,
},
sort: if should_sort {
CompletionSort::Alphabetical
} else {
CompletionSort::Smart
},
sort: completion_options.sort,
});
}
@ -124,9 +119,17 @@ impl Completer for CustomCompletion {
let options = custom_completion_options.unwrap_or(completion_options.clone());
let mut matcher = NuMatcher::new(String::from_utf8_lossy(prefix), options);
if should_sort {
for sugg in suggestions {
matcher.add_semantic_suggestion(sugg);
}
matcher.results()
} else {
suggestions
.into_iter()
.filter(|sugg| matcher.matches(&sugg.suggestion.value))
.collect()
}
}
}

View file

@ -88,6 +88,27 @@ fn completer_strings_with_options() -> NuCompleter {
NuCompleter::new(Arc::new(engine), Arc::new(stack))
}
#[fixture]
fn completer_strings_no_sort() -> NuCompleter {
// Create a new engine
let (_, _, mut engine, mut stack) = new_engine();
let command = r#"
def animals [] {
{
completions: ["zzzfoo", "foo", "not matched", "abcfoo" ],
options: {
completion_algorithm: "fuzzy",
sort: false,
}
}
}
def my-command [animal: string@animals] { print $animal }"#;
assert!(support::merge_input(command.as_bytes(), &mut engine, &mut stack).is_ok());
// Instantiate a new completer
NuCompleter::new(Arc::new(engine), Arc::new(stack))
}
#[fixture]
fn custom_completer() -> NuCompleter {
// Create a new engine
@ -210,6 +231,13 @@ fn customcompletions_case_insensitive(mut completer_strings_with_options: NuComp
match_suggestions(&expected, &suggestions);
}
#[rstest]
fn customcompletions_no_sort(mut completer_strings_no_sort: NuCompleter) {
let suggestions = completer_strings_no_sort.complete("my-command foo", 14);
let expected: Vec<String> = vec!["zzzfoo".into(), "foo".into(), "abcfoo".into()];
match_suggestions(&expected, &suggestions);
}
#[test]
fn dotnu_completions() {
// Create a new engine