nushell/crates/nu-command/tests/main.rs

248 lines
7.0 KiB
Rust
Raw Normal View History

2023-06-14 23:12:55 +02:00
use nu_protocol::{
engine::{EngineState, StateWorkingSet},
Category, PositionalArg, Span,
2023-06-14 23:12:55 +02:00
};
use quickcheck_macros::quickcheck;
mod commands;
mod format_conversions;
Rework sorting and add cell path and closure comparators to `sort-by` (#13154) # Description Closes #12535 Implements sort-by functionality of #8322 Fixes sort-by part of #8667 This PR does two main things: add a new cell path and closure parameter to `sort-by`, and attempt to make Nushell's sorting behavior well-defined. ## `sort-by` features The `columns` parameter is replaced with a `comparator` parameter, which can be a cell path or a closure. Examples are from docs PR. 1. Cell paths The basic interactive usage of `sort-by` is the same. For example, `ls | sort-by modified` still works the same as before. It is not quite a drop-in replacement, see [behavior changes](#behavior-changes). Here's an example of how the cell path comparator might be useful: ```nu > let cities = [ {name: 'New York', info: { established: 1624, population: 18_819_000 } } {name: 'Kyoto', info: { established: 794, population: 37_468_000 } } {name: 'São Paulo', info: { established: 1554, population: 21_650_000 } } ] > $cities | sort-by info.established ╭───┬───────────┬────────────────────────────╮ │ # │ name │ info │ ├───┼───────────┼────────────────────────────┤ │ 0 │ Kyoto │ ╭─────────────┬──────────╮ │ │ │ │ │ established │ 794 │ │ │ │ │ │ population │ 37468000 │ │ │ │ │ ╰─────────────┴──────────╯ │ │ 1 │ São Paulo │ ╭─────────────┬──────────╮ │ │ │ │ │ established │ 1554 │ │ │ │ │ │ population │ 21650000 │ │ │ │ │ ╰─────────────┴──────────╯ │ │ 2 │ New York │ ╭─────────────┬──────────╮ │ │ │ │ │ established │ 1624 │ │ │ │ │ │ population │ 18819000 │ │ │ │ │ ╰─────────────┴──────────╯ │ ╰───┴───────────┴────────────────────────────╯ ``` 2. Key closures You can supply a closure which will transform each value into a sorting key (without changing the underlying data). Here's an example of a key closure, where we want to sort a list of assignments by their average grade: ```nu > let assignments = [ {name: 'Homework 1', grades: [97 89 86 92 89] } {name: 'Homework 2', grades: [91 100 60 82 91] } {name: 'Exam 1', grades: [78 88 78 53 90] } {name: 'Project', grades: [92 81 82 84 83] } ] > $assignments | sort-by { get grades | math avg } ╭───┬────────────┬───────────────────────╮ │ # │ name │ grades │ ├───┼────────────┼───────────────────────┤ │ 0 │ Exam 1 │ [78, 88, 78, 53, 90] │ │ 1 │ Project │ [92, 81, 82, 84, 83] │ │ 2 │ Homework 2 │ [91, 100, 60, 82, 91] │ │ 3 │ Homework 1 │ [97, 89, 86, 92, 89] │ ╰───┴────────────┴───────────────────────╯ ``` 3. Custom sort closure The `--custom`, or `-c`, flag will tell `sort-by` to interpret closures as custom sort closures. A custom sort closure has two parameters, and returns a boolean. The closure should return `true` if the first parameter comes _before_ the second parameter in the sort order. For a simple example, we could rewrite a cell path sort as a custom sort (see [here](https://github.com/nushell/nushell.github.io/pull/1568/files#diff-a7a233e66a361d8665caf3887eb71d4288000001f401670c72b95cc23a948e86R231) for a more complex example): ```nu > ls | sort-by -c {|a, b| $a.size < $b.size } ╭───┬─────────────────────┬──────┬──────────┬────────────────╮ │ # │ name │ type │ size │ modified │ ├───┼─────────────────────┼──────┼──────────┼────────────────┤ │ 0 │ my-secret-plans.txt │ file │ 100 B │ 10 minutes ago │ │ 1 │ shopping_list.txt │ file │ 100 B │ 2 months ago │ │ 2 │ myscript.nu │ file │ 1.1 KiB │ 2 weeks ago │ │ 3 │ bigfile.img │ file │ 10.0 MiB │ 3 weeks ago │ ╰───┴─────────────────────┴──────┴──────────┴────────────────╯ ``` ## Making sort more consistent I think it's important for something as essential as `sort` to have well-defined semantics. This PR contains some changes to try to make the behavior of `sort` and `sort-by` consistent. In addition, after working with the internals of sorting code, I have a much deeper understanding of all of the edge cases. Here is my attempt to try to better define some of the semantics of sorting (if you are just interested in changes, skip to "User-Facing changes") - `sort`, `sort -v`, and `sort-by` now all work the same. Each individual sort implementation has been refactored into two functions in `sort_utils.rs`: `sort`, and `sort_by`. These can also be used in other parts of Nushell where values need to be sorted. - `sort` and `sort-by` used to handle `-i` and `-n` differently. - `sort -n` would consider all values which can't be coerced into a string to be equal - `sort-by -i` and `sort-by -n` would only work if all values were strings - In this PR, insensitive sort only affects comparison between strings, and natural sort only applies to numbers and strings (see below). - (not a change) Before and after this PR, `sort` and `sort-by` support sorting mixed types. There was a lot of discussion about potentially making `sort` and `sort-by` only work on lists of homogeneous types, but the general consensus was that `sort` should not error just because its input contains incompatible types. - In order to try to make working with data containing `null` values easier, I changed the PartialOrd order to sort `Nothing` values to the end of a list, regardless of what other types the list contains. Before, `null` would be sorted before `Binary`, `CellPath`, and `Custom` values. - (not a change) When sorted, lists of mixed types will contain sorted values of each type in order, for the most part - (not a change) For example, `[0x[1] (date now) "a" ("yesterday" | into datetime) "b" 0x[0]]` will be sorted as `["a", "b", a day ago, now, [0], [1]]`, where sorted strings appear first, then sorted datetimes, etc. - (not a change) The exception to this is `Int`s and `Float`s, which will intermix, `Strings` and `Glob`s, which will intermix, and `None` as described above. Additionally, natural sort will intermix strings with ints and floats (see below). - Natural sort no longer coerce all inputs to strings. - I did originally make natural only apply to strings, but @fdncred pointed out that the previous behavior also allowed you to sort numeric strings with numbers. This seems like a useful feature if we are trying to support sorting with mixed types, so I settled on coercing only numbers (int, float). This can be reverted if people don't like it. - Here is an example of this behavior in action, which is the same before and after this PR: ```nushell $ [1 "4" 3 "2"] | sort --natural ╭───┬───╮ │ 0 │ 1 │ │ 1 │ 2 │ │ 2 │ 3 │ │ 3 │ 4 │ ╰───┴───╯ ``` # User-Facing Changes ## New features - Replaces the `columns` string parameter of `sort-by` with a cell path or a closure. - The cell path parameter works exactly as you would expect - By default, the `closure` parameter acts as a "key sort"; that is, each element is transformed by the closure into a sorting key - With the `--custom` (`-c`) parameter, you can define a comparison function for completely custom sorting order. ## Behavior changes <details> <summary><code>sort -v</code> does not coerce record values to strings</summary> This was a bit of a surprising behavior, and is now unified with the behavior of `sort` and `sort-by`. Here's an example where you can observe the values being implicitly coerced into strings for sorting, as they are sorted like strings rather than numbers: Old behavior: ```nushell $ {foo: 9 bar: 10} | sort -v ╭─────┬────╮ │ bar │ 10 │ │ foo │ 9 │ ╰─────┴────╯ ``` New behavior: ```nushell $ {foo: 9 bar: 10} | sort -v ╭─────┬────╮ │ foo │ 9 │ │ bar │ 10 │ ╰─────┴────╯ ``` </details> <details> <summary>Changed <code>sort-by</code> parameters from <code>string</code> to <code>cell-path</code> or <code>closure</code>. Typical interactive usage is the same as before, but if passing a variable to <code>sort-by</code> it must be a cell path (or closure), not a string</summary> Old behavior: ```nushell $ let sort = "modified" $ ls | sort-by $sort ╭───┬──────┬──────┬──────┬────────────────╮ │ # │ name │ type │ size │ modified │ ├───┼──────┼──────┼──────┼────────────────┤ │ 0 │ foo │ file │ 0 B │ 10 hours ago │ │ 1 │ bar │ file │ 0 B │ 35 seconds ago │ ╰───┴──────┴──────┴──────┴────────────────╯ ``` New behavior: ```nushell $ let sort = "modified" $ ls | sort-by $sort Error: nu::shell::type_mismatch × Type mismatch. ╭─[entry #10:1:14] 1 │ ls | sort-by $sort · ──┬── · ╰── Cannot sort using a value which is not a cell path or closure ╰──── $ let sort = $."modified" $ ls | sort-by $sort ╭───┬──────┬──────┬──────┬───────────────╮ │ # │ name │ type │ size │ modified │ ├───┼──────┼──────┼──────┼───────────────┤ │ 0 │ foo │ file │ 0 B │ 10 hours ago │ │ 1 │ bar │ file │ 0 B │ 2 minutes ago │ ╰───┴──────┴──────┴──────┴───────────────╯ ``` </details> <details> <summary>Insensitve and natural sorting behavior reworked</summary> Previously, the `-i` and `-n` worked differently for `sort` and `sort-by` (see "Making sort more consistent"). Here are examples of how these options result in different sorts now: 1. `sort -n` - Old behavior (types other than numbers, strings, dates, and binary sorted incorrectly) ```nushell $ [2sec 1sec] | sort -n ╭───┬──────╮ │ 0 │ 2sec │ │ 1 │ 1sec │ ╰───┴──────╯ ``` - New behavior ```nushell $ [2sec 1sec] | sort -n ╭───┬──────╮ │ 0 │ 1sec │ │ 1 │ 2sec │ ╰───┴──────╯ ``` 2. `sort-by -i` - Old behavior (uppercase words appear before lowercase words as they would in a typical sort, indicating this is not actually an insensitive sort) ```nushell $ ["BAR" "bar" "foo" 2 "FOO" 1] | wrap a | sort-by -i a ╭───┬─────╮ │ # │ a │ ├───┼─────┤ │ 0 │ 1 │ │ 1 │ 2 │ │ 2 │ BAR │ │ 3 │ FOO │ │ 4 │ bar │ │ 5 │ foo │ ╰───┴─────╯ ``` - New behavior (strings are sorted stably, indicating this is an insensitive sort) ```nushell $ ["BAR" "bar" "foo" 2 "FOO" 1] | wrap a | sort-by -i a ╭───┬─────╮ │ # │ a │ ├───┼─────┤ │ 0 │ 1 │ │ 1 │ 2 │ │ 2 │ BAR │ │ 3 │ bar │ │ 4 │ foo │ │ 5 │ FOO │ ╰───┴─────╯ ``` 3. `sort-by -n` - Old behavior (natural sort does not work when data contains non-string values) ```nushell $ ["10" 8 "9"] | wrap a | sort-by -n a ╭───┬────╮ │ # │ a │ ├───┼────┤ │ 0 │ 8 │ │ 1 │ 10 │ │ 2 │ 9 │ ╰───┴────╯ ``` - New behavior ```nushell $ ["10" 8 "9"] | wrap a | sort-by -n a ╭───┬────╮ │ # │ a │ ├───┼────┤ │ 0 │ 8 │ │ 1 │ 9 │ │ 2 │ 10 │ ╰───┴────╯ ``` </details> <details> <summary> Sorting a list of non-record values with a non-existent column/path now errors instead of sorting the values directly (<code>sort</code> should be used for this, not <code>sort-by</code>) </summary> Old behavior: ```nushell $ [2 1] | sort-by foo ╭───┬───╮ │ 0 │ 1 │ │ 1 │ 2 │ ╰───┴───╯ ``` New behavior: ```nushell $ [2 1] | sort-by foo Error: nu::shell::incompatible_path_access × Data cannot be accessed with a cell path ╭─[entry #29:1:17] 1 │ [2 1] | sort-by foo · ─┬─ · ╰── int doesn't support cell paths ╰──── ``` </details> <details> <summary><code>sort</code> and <code>sort-by</code> output <code>List</code> instead of <code>ListStream</code> </summary> This isn't a meaningful change (unless I misunderstand the purpose of ListStream), since `sort` and `sort-by` both need to collect in order to do the sorting anyway, but is user observable. Old behavior: ```nushell $ ls | sort | describe -d ╭──────────┬───────────────────╮ │ type │ stream │ │ origin │ nushell │ │ subtype │ {record 3 fields} │ │ metadata │ {record 1 field} │ ╰──────────┴───────────────────╯ ``` ```nushell $ ls | sort-by name | describe -d ╭──────────┬───────────────────╮ │ type │ stream │ │ origin │ nushell │ │ subtype │ {record 3 fields} │ │ metadata │ {record 1 field} │ ╰──────────┴───────────────────╯ ``` New behavior: ```nushell ls | sort | describe -d ╭────────┬─────────────────╮ │ type │ list │ │ length │ 22 │ │ values │ [table 22 rows] │ ╰────────┴─────────────────╯ ``` ```nushell $ ls | sort-by name | describe -d ╭────────┬─────────────────╮ │ type │ list │ │ length │ 22 │ │ values │ [table 22 rows] │ ╰────────┴─────────────────╯ ``` </details> - `sort` now errors when nothing is piped in (`sort-by` already did this) # Tests + Formatting I added lots of unit tests on the new sort implementation to enforce new sort behaviors and prevent regressions. # After Submitting See [docs PR](https://github.com/nushell/nushell.github.io/pull/1568), which is ~2/3 finished. --------- Co-authored-by: NotTheDr01ds <32344964+NotTheDr01ds@users.noreply.github.com> Co-authored-by: Ian Manske <ian.manske@pm.me>
2024-10-10 04:18:16 +02:00
mod sort_utils;
2023-06-14 23:12:55 +02:00
fn create_default_context() -> EngineState {
nu_command::add_shell_command_context(nu_cmd_lang::create_default_context())
}
#[quickcheck]
fn quickcheck_parse(data: String) -> bool {
let (tokens, err) = nu_parser::lex(data.as_bytes(), 0, b"", b"", true);
if err.is_none() {
let context = create_default_context();
{
let mut working_set = StateWorkingSet::new(&context);
let _ = working_set.add_file("quickcheck".into(), data.as_bytes());
Reuse the cached parse results of parsed files (#8949) # Description This does a lookup in the cache of parsed files to see if a span can be found for a file that was previously loaded with the same contents, then uses that span to find the parsed block for that file. The end result should, in theory, be identical but doesn't require any reparsing or creating new blocks/new definitions that aren't needed. This drops the sg.nu benchmark from: ``` ╭───┬───────────────────╮ │ 0 │ 280ms 606µs 208ns │ │ 1 │ 282ms 654µs 416ns │ │ 2 │ 252ms 640µs 541ns │ │ 3 │ 250ms 940µs 41ns │ │ 4 │ 241ms 216µs 375ns │ │ 5 │ 257ms 310µs 583ns │ │ 6 │ 196ms 739µs 416ns │ ╰───┴───────────────────╯ ``` to: ``` ╭───┬───────────────────╮ │ 0 │ 118ms 698µs 125ns │ │ 1 │ 121ms 327µs │ │ 2 │ 121ms 873µs 500ns │ │ 3 │ 124ms 94µs 708ns │ │ 4 │ 113ms 733µs 291ns │ │ 5 │ 108ms 663µs 125ns │ │ 6 │ 63ms 482µs 625ns │ ╰───┴───────────────────╯ ``` I was hoping to also see some startup time improvements, but I didn't notice much there. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # 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 -A clippy::needless_collect` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass - `cargo run -- crates/nu-std/tests/run.nu` 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 > ``` --> # 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. -->
2023-04-21 21:00:33 +02:00
let _ =
nu_parser::parse_block(&mut working_set, &tokens, Span::new(0, 0), false, false);
}
}
true
}
#[test]
fn arguments_end_period() {
fn ends_period(cmd_name: &str, ty: &str, arg: PositionalArg, failures: &mut Vec<String>) {
let arg_name = arg.name;
let desc = arg.desc;
if !desc.ends_with('.') {
failures.push(format!(
"{cmd_name} {ty} argument \"{arg_name}\": \"{desc}\""
));
}
}
let ctx = crate::create_default_context();
let decls = ctx.get_decls_sorted(true);
let mut failures = Vec::new();
for (name_bytes, decl_id) in decls {
let cmd = ctx.get_decl(decl_id);
let cmd_name = String::from_utf8_lossy(&name_bytes);
let signature = cmd.signature();
for arg in signature.required_positional {
ends_period(&cmd_name, "required", arg, &mut failures);
}
for arg in signature.optional_positional {
ends_period(&cmd_name, "optional", arg, &mut failures);
}
if let Some(arg) = signature.rest_positional {
ends_period(&cmd_name, "rest", arg, &mut failures);
}
}
assert!(
failures.is_empty(),
"Command argument description does not end with a period:\n{}",
failures.join("\n")
);
}
#[test]
fn arguments_start_uppercase() {
fn starts_uppercase(cmd_name: &str, ty: &str, arg: PositionalArg, failures: &mut Vec<String>) {
let arg_name = arg.name;
let desc = arg.desc;
// Check lowercase to allow usage to contain syntax like:
//
// "`as` keyword …"
if desc.starts_with(|u: char| u.is_lowercase()) {
failures.push(format!(
"{cmd_name} {ty} argument \"{arg_name}\": \"{desc}\""
));
}
}
let ctx = crate::create_default_context();
let decls = ctx.get_decls_sorted(true);
let mut failures = Vec::new();
for (name_bytes, decl_id) in decls {
let cmd = ctx.get_decl(decl_id);
let cmd_name = String::from_utf8_lossy(&name_bytes);
let signature = cmd.signature();
for arg in signature.required_positional {
starts_uppercase(&cmd_name, "required", arg, &mut failures);
}
for arg in signature.optional_positional {
starts_uppercase(&cmd_name, "optional", arg, &mut failures);
}
if let Some(arg) = signature.rest_positional {
starts_uppercase(&cmd_name, "rest", arg, &mut failures);
}
}
assert!(
failures.is_empty(),
"Command argument description does not end with a period:\n{}",
failures.join("\n")
);
}
#[test]
fn signature_name_matches_command_name() {
2023-06-14 23:12:55 +02:00
let ctx = create_default_context();
2022-12-30 16:44:37 +01:00
let decls = ctx.get_decls_sorted(true);
let mut failures = Vec::new();
2022-12-30 16:44:37 +01:00
for (name_bytes, decl_id) in decls {
let cmd = ctx.get_decl(decl_id);
2022-12-30 16:44:37 +01:00
let cmd_name = String::from_utf8_lossy(&name_bytes);
let sig_name = cmd.signature().name;
let category = cmd.signature().category;
if cmd_name != sig_name {
failures.push(format!(
"{cmd_name} ({category:?}): Signature name \"{sig_name}\" is not equal to the command name \"{cmd_name}\""
));
}
}
assert!(
failures.is_empty(),
"Name mismatch:\n{}",
failures.join("\n")
);
}
#[test]
fn commands_declare_input_output_types() {
2023-06-14 23:12:55 +02:00
let ctx = create_default_context();
2022-12-30 16:44:37 +01:00
let decls = ctx.get_decls_sorted(true);
let mut failures = Vec::new();
2022-12-30 16:44:37 +01:00
for (_, decl_id) in decls {
let cmd = ctx.get_decl(decl_id);
let sig_name = cmd.signature().name;
let category = cmd.signature().category;
if matches!(category, Category::Removed) {
// Deprecated/Removed commands don't have to conform
continue;
}
if cmd.signature().input_output_types.is_empty() {
failures.push(format!(
"{sig_name} ({category:?}): No pipeline input/output type signatures found"
));
}
}
assert!(
failures.is_empty(),
"Command missing type annotations:\n{}",
failures.join("\n")
);
}
#[test]
fn no_search_term_duplicates() {
let ctx = crate::create_default_context();
2022-12-30 16:44:37 +01:00
let decls = ctx.get_decls_sorted(true);
let mut failures = Vec::new();
2022-12-30 16:44:37 +01:00
for (name_bytes, decl_id) in decls {
let cmd = ctx.get_decl(decl_id);
2022-12-30 16:44:37 +01:00
let cmd_name = String::from_utf8_lossy(&name_bytes);
let search_terms = cmd.search_terms();
let category = cmd.signature().category;
for search_term in search_terms {
if cmd_name.contains(search_term) {
failures.push(format!("{cmd_name} ({category:?}): Search term \"{search_term}\" is substring of command name \"{cmd_name}\""));
}
}
}
assert!(
failures.is_empty(),
"Duplication in search terms:\n{}",
failures.join("\n")
);
}
#[test]
fn description_end_period() {
let ctx = crate::create_default_context();
let decls = ctx.get_decls_sorted(true);
let mut failures = Vec::new();
for (name_bytes, decl_id) in decls {
let cmd = ctx.get_decl(decl_id);
let cmd_name = String::from_utf8_lossy(&name_bytes);
let description = cmd.description();
if !description.ends_with('.') {
failures.push(format!("{cmd_name}: \"{description}\""));
}
}
assert!(
failures.is_empty(),
"Command description does not end with a period:\n{}",
failures.join("\n")
);
}
#[test]
fn description_start_uppercase() {
let ctx = crate::create_default_context();
let decls = ctx.get_decls_sorted(true);
let mut failures = Vec::new();
for (name_bytes, decl_id) in decls {
let cmd = ctx.get_decl(decl_id);
let cmd_name = String::from_utf8_lossy(&name_bytes);
let description = cmd.description();
// Check lowercase to allow description to contain syntax like:
//
// "`$env.FOO = ...`"
if description.starts_with(|u: char| u.is_lowercase()) {
failures.push(format!("{cmd_name}: \"{description}\""));
}
}
assert!(
failures.is_empty(),
"Command description does not start with an uppercase letter:\n{}",
failures.join("\n")
);
}