diff --git a/crates/nu-command/src/filters/sort_by.rs b/crates/nu-command/src/filters/sort_by.rs index 602de86c10..7ac14590fa 100644 --- a/crates/nu-command/src/filters/sort_by.rs +++ b/crates/nu-command/src/filters/sort_by.rs @@ -1,10 +1,9 @@ -use chrono::{DateTime, FixedOffset}; use nu_engine::{column::column_does_not_exist, CallExt}; use nu_protocol::{ ast::Call, engine::{Command, EngineState, Stack}, - Category, Config, Example, IntoInterruptiblePipelineData, PipelineData, ShellError, Signature, - Span, SyntaxShape, Value, + Category, Example, IntoInterruptiblePipelineData, PipelineData, ShellError, Signature, Span, + SyntaxShape, Value, }; use std::cmp::Ordering; @@ -112,10 +111,9 @@ impl Command for SortBy { let reverse = call.has_flag("reverse"); let insensitive = call.has_flag("insensitive"); let metadata = &input.metadata(); - let config = stack.get_config()?; let mut vec: Vec<_> = input.into_iter().collect(); - sort(&mut vec, columns, call, insensitive, &config)?; + sort(&mut vec, columns, call.head, insensitive)?; if reverse { vec.reverse() @@ -134,9 +132,8 @@ impl Command for SortBy { pub fn sort( vec: &mut [Value], columns: Vec, - call: &Call, + span: Span, insensitive: bool, - config: &Config, ) -> Result<(), ShellError> { if vec.is_empty() { return Err(ShellError::LabeledError( @@ -153,11 +150,11 @@ pub fn sort( } => { if columns.is_empty() { println!("sort-by requires a column name to sort table data"); - return Err(ShellError::CantFindColumn(call.head, call.head)); + return Err(ShellError::CantFindColumn(span, span)); } if column_does_not_exist(columns.clone(), cols.to_vec()) { - return Err(ShellError::CantFindColumn(call.head, call.head)); + return Err(ShellError::CantFindColumn(span, span)); } // check to make sure each value in each column in the record @@ -180,33 +177,32 @@ pub fn sort( .iter() .all(|x| matches!(x.get_type(), nu_protocol::Type::String)); - vec.sort_by(|a, b| { - process( - a, - b, - &columns[0], - call, - should_sort_case_insensitively, - config, - ) - .expect("sort_by Value::Record bug") - }); + vec.sort_by(|a, b| process(a, b, &columns, span, should_sort_case_insensitively)); } _ => { vec.sort_by(|a, b| { if insensitive { - let lowercase_left = Value::string( - a.into_string("", config).to_ascii_lowercase(), - Span::test_data(), - ); - let lowercase_right = Value::string( - b.into_string("", config).to_ascii_lowercase(), - Span::test_data(), - ); - coerce_compare(&lowercase_left, &lowercase_right, call) - .expect("sort_by default bug") + let lowercase_left = match a { + Value::String { val, span } => Value::String { + val: val.to_ascii_lowercase(), + span: *span, + }, + _ => a.clone(), + }; + + let lowercase_right = match b { + Value::String { val, span } => Value::String { + val: val.to_ascii_lowercase(), + span: *span, + }, + _ => b.clone(), + }; + + lowercase_left + .partial_cmp(&lowercase_right) + .unwrap_or(Ordering::Equal) } else { - coerce_compare(a, b, call).expect("sort_by default bug") + a.partial_cmp(b).unwrap_or(Ordering::Equal) } }); } @@ -217,150 +213,53 @@ pub fn sort( pub fn process( left: &Value, right: &Value, - column: &str, - call: &Call, + columns: &[String], + span: Span, insensitive: bool, - config: &Config, -) -> Result { - let left_value = left.get_data_by_key(column); +) -> Ordering { + for column in columns { + let left_value = left.get_data_by_key(column); - let left_res = match left_value { - Some(left_res) => left_res, - None => Value::Nothing { span: call.head }, - }; + let left_res = match left_value { + Some(left_res) => left_res, + None => Value::Nothing { span }, + }; - let right_value = right.get_data_by_key(column); + let right_value = right.get_data_by_key(column); - let right_res = match right_value { - Some(right_res) => right_res, - None => Value::Nothing { span: call.head }, - }; + let right_res = match right_value { + Some(right_res) => right_res, + None => Value::Nothing { span }, + }; - if insensitive { - let lowercase_left = Value::string( - left_res.into_string("", config).to_ascii_lowercase(), - Span::test_data(), - ); - let lowercase_right = Value::string( - right_res.into_string("", config).to_ascii_lowercase(), - Span::test_data(), - ); - coerce_compare(&lowercase_left, &lowercase_right, call) - } else { - coerce_compare(&left_res, &right_res, call) - } -} + let result = if insensitive { + let lowercase_left = match left_res { + Value::String { val, span } => Value::String { + val: val.to_ascii_lowercase(), + span, + }, + _ => left_res, + }; -#[derive(Debug)] -pub enum CompareValues { - Ints(i64, i64), - Floats(f64, f64), - String(String, String), - Booleans(bool, bool), - Filesize(i64, i64), - Date(DateTime, DateTime), -} - -impl CompareValues { - pub fn compare(&self) -> std::cmp::Ordering { - match self { - CompareValues::Ints(left, right) => left.cmp(right), - CompareValues::Floats(left, right) => process_floats(left, right), - CompareValues::String(left, right) => left.cmp(right), - CompareValues::Booleans(left, right) => left.cmp(right), - CompareValues::Filesize(left, right) => left.cmp(right), - CompareValues::Date(left, right) => left.cmp(right), + let lowercase_right = match right_res { + Value::String { val, span } => Value::String { + val: val.to_ascii_lowercase(), + span, + }, + _ => right_res, + }; + lowercase_left + .partial_cmp(&lowercase_right) + .unwrap_or(Ordering::Equal) + } else { + left_res.partial_cmp(&right_res).unwrap_or(Ordering::Equal) + }; + if result != Ordering::Equal { + return result; } } -} -pub fn process_floats(left: &f64, right: &f64) -> std::cmp::Ordering { - let result = left.partial_cmp(right); - match result { - Some(Ordering::Greater) => Ordering::Greater, - Some(Ordering::Less) => Ordering::Less, - _ => Ordering::Equal, - } -} - -/* -Arbitrary Order of Values: - Floats - Ints - Strings - Bools - Lists -*/ - -pub fn coerce_compare(left: &Value, right: &Value, call: &Call) -> Result { - Ok(match (left, right) { - (Value::Float { val: left, .. }, Value::Float { val: right, .. }) => { - CompareValues::Floats(*left, *right).compare() - } - (Value::Filesize { val: left, .. }, Value::Filesize { val: right, .. }) => { - CompareValues::Filesize(*left, *right).compare() - } - (Value::Date { val: left, .. }, Value::Date { val: right, .. }) => { - CompareValues::Date(*left, *right).compare() - } - (Value::Int { val: left, .. }, Value::Int { val: right, .. }) => { - CompareValues::Ints(*left, *right).compare() - } - (Value::String { val: left, .. }, Value::String { val: right, .. }) => { - CompareValues::String(left.clone(), right.clone()).compare() - } - (Value::Bool { val: left, .. }, Value::Bool { val: right, .. }) => { - CompareValues::Booleans(*left, *right).compare() - } - - // FIXME: Not sure how to compare and sort lists - (Value::List { .. }, Value::List { .. }) => Ordering::Equal, - - // Floats will always come before Ints - (Value::Float { .. }, Value::Int { .. }) => Ordering::Less, - (Value::Int { .. }, Value::Float { .. }) => Ordering::Greater, - - // Floats will always come before Strings - (Value::Float { .. }, Value::String { .. }) => Ordering::Less, - (Value::String { .. }, Value::Float { .. }) => Ordering::Greater, - - // Floats will always come before Bools - (Value::Float { .. }, Value::Bool { .. }) => Ordering::Less, - (Value::Bool { .. }, Value::Float { .. }) => Ordering::Greater, - - // Floats will always come before Lists - (Value::Float { .. }, Value::List { .. }) => Ordering::Less, - (Value::List { .. }, Value::Float { .. }) => Ordering::Greater, - - // Ints will always come before strings - (Value::Int { .. }, Value::String { .. }) => Ordering::Less, - (Value::String { .. }, Value::Int { .. }) => Ordering::Greater, - - // Ints will always come before Bools - (Value::Int { .. }, Value::Bool { .. }) => Ordering::Less, - (Value::Bool { .. }, Value::Int { .. }) => Ordering::Greater, - - // Ints will always come before Lists - (Value::Int { .. }, Value::List { .. }) => Ordering::Less, - (Value::List { .. }, Value::Int { .. }) => Ordering::Greater, - - // Strings will always come before Bools - (Value::String { .. }, Value::Bool { .. }) => Ordering::Less, - (Value::Bool { .. }, Value::String { .. }) => Ordering::Greater, - - // Strings will always come before Lists - (Value::String { .. }, Value::List { .. }) => Ordering::Less, - (Value::List { .. }, Value::String { .. }) => Ordering::Greater, - - // Bools will always come before Lists - (Value::Bool { .. }, Value::List { .. }) => Ordering::Less, - (Value::List { .. }, Value::Bool { .. }) => Ordering::Greater, - - _ => { - let description = format!("not able to compare {:?} with {:?}\n", left, right); - return Err(ShellError::TypeMismatch(description, call.head)); - } - }) + Ordering::Equal } #[cfg(test)] diff --git a/crates/nu-command/tests/commands/sort_by.rs b/crates/nu-command/tests/commands/sort_by.rs index 7415b33f10..08dcd923ef 100644 --- a/crates/nu-command/tests/commands/sort_by.rs +++ b/crates/nu-command/tests/commands/sort_by.rs @@ -127,7 +127,7 @@ fn ls_sort_by_type_name_sensitive() { "# )); - let json_output = r#"[{"name": "C","type": "Dir"},{"name": "a.txt","type": "File"},{"name": "B.txt","type": "File"}]"#; + let json_output = r#"[{"name": "C","type": "Dir"},{"name": "B.txt","type": "File"},{"name": "a.txt","type": "File"}]"#; assert_eq!(actual.out, json_output); } diff --git a/src/tests/test_strings.rs b/src/tests/test_strings.rs index 3e542fd71e..4e95c48947 100644 --- a/src/tests/test_strings.rs +++ b/src/tests/test_strings.rs @@ -89,3 +89,19 @@ fn single_tick_interpolation() -> TestResult { fn detect_newlines() -> TestResult { run_test("'hello\r\nworld' | lines | get 0 | str length", "5") } + +#[test] +fn case_insensitive_sort() -> TestResult { + run_test( + r#"[a, B, d, C, f] | sort-by -i | to json --raw"#, + "[\"a\",\"B\",\"C\",\"d\",\"f\"]", + ) +} + +#[test] +fn case_insensitive_sort_columns() -> TestResult { + run_test( + r#"[[version, package]; ["two", "Abc"], ["three", "abc"], ["four", "abc"]] | sort-by -i package version | to json --raw"#, + r#"[{"version": "four","package": "abc"},{"version": "three","package": "abc"},{"version": "two","package": "Abc"}]"#, + ) +}