diff --git a/crates/nu-command/src/filters/move_.rs b/crates/nu-command/src/filters/move_.rs index 849c07392a..bc8e6dca95 100644 --- a/crates/nu-command/src/filters/move_.rs +++ b/crates/nu-command/src/filters/move_.rs @@ -194,35 +194,25 @@ fn move_record_columns( ) -> Result { let mut column_idx: Vec = Vec::with_capacity(columns.len()); - // Check if before/after column exist - match &before_or_after.item { - BeforeOrAfter::After(after) => { - if !record.contains(after) { - return Err(ShellError::GenericError { - error: "Cannot move columns".into(), - msg: "column does not exist".into(), - span: Some(before_or_after.span), - help: None, - inner: vec![], - }); - } - } - BeforeOrAfter::Before(before) => { - if !record.contains(before) { - return Err(ShellError::GenericError { - error: "Cannot move columns".into(), - msg: "column does not exist".into(), - span: Some(before_or_after.span), - help: None, - inner: vec![], - }); - } - } + let pivot = match &before_or_after.item { + BeforeOrAfter::Before(before) => before, + BeforeOrAfter::After(after) => after, + }; + + // check if pivot exists + if !record.contains(pivot) { + return Err(ShellError::GenericError { + error: "Cannot move columns".into(), + msg: "column does not exist".into(), + span: Some(before_or_after.span), + help: None, + inner: vec![], + }); } // Find indices of columns to be moved for column in columns.iter() { - if let Some(idx) = record.index_of(&column.coerce_str()?) { + if let Some(idx) = record.index_of(column.coerce_string()?) { column_idx.push(idx); } else { return Err(ShellError::GenericError { @@ -233,47 +223,44 @@ fn move_record_columns( inner: vec![], }); } + + let column_as_string = column.coerce_string()?; + // check if column is also pivot + if &column_as_string == pivot { + return Err(ShellError::IncompatibleParameters { + left_message: "Column cannot be moved".to_string(), + left_span: column.span(), + right_message: "relative to itself".to_string(), + right_span: before_or_after.span, + }); + } } let mut out = Record::with_capacity(record.len()); for (i, (inp_col, inp_val)) in record.iter().enumerate() { - match &before_or_after.item { - BeforeOrAfter::After(after) if after == inp_col => { - out.push(inp_col.clone(), inp_val.clone()); - - for idx in column_idx.iter() { - if let Some((col, val)) = record.get_index(*idx) { - out.push(col.clone(), val.clone()); - } else { - return Err(ShellError::NushellFailedSpanned { - msg: "Error indexing input columns".to_string(), - label: "originates from here".to_string(), - span, - }); - } - } - } - BeforeOrAfter::Before(before) if before == inp_col => { - for idx in column_idx.iter() { - if let Some((col, val)) = record.get_index(*idx) { - out.push(col.clone(), val.clone()); - } else { - return Err(ShellError::NushellFailedSpanned { - msg: "Error indexing input columns".to_string(), - label: "originates from here".to_string(), - span, - }); - } - } - + if inp_col == pivot { + if matches!(&before_or_after.item, BeforeOrAfter::After(..)) { out.push(inp_col.clone(), inp_val.clone()); } - _ => { - if !column_idx.contains(&i) { - out.push(inp_col.clone(), inp_val.clone()); + + for idx in column_idx.iter() { + if let Some((col, val)) = record.get_index(*idx) { + out.push(col.clone(), val.clone()); + } else { + return Err(ShellError::NushellFailedSpanned { + msg: "Error indexing input columns".to_string(), + label: "originates from here".to_string(), + span, + }); } } + + if matches!(&before_or_after.item, BeforeOrAfter::Before(..)) { + out.push(inp_col.clone(), inp_val.clone()); + } + } else if !column_idx.contains(&i) { + out.push(inp_col.clone(), inp_val.clone()); } } @@ -284,10 +271,194 @@ fn move_record_columns( mod test { use super::*; + // helper + fn get_test_record(columns: Vec<&str>, values: Vec) -> Record { + let test_span = Span::test_data(); + Record::from_raw_cols_vals( + columns.iter().map(|col| col.to_string()).collect(), + values.iter().map(|val| Value::test_int(*val)).collect(), + test_span, + test_span, + ) + .unwrap() + } + #[test] fn test_examples() { use crate::test_examples; test_examples(Move {}) } + + #[test] + fn move_after_with_single_column() { + let test_span = Span::test_data(); + let test_record = get_test_record(vec!["a", "b", "c", "d"], vec![1, 2, 3, 4]); + let after: Spanned = Spanned { + item: BeforeOrAfter::After("c".to_string()), + span: test_span, + }; + let columns = [Value::test_string("a")]; + + // corresponds to: {a: 1, b: 2, c: 3, d: 4} | move a --after c + let result = move_record_columns(&test_record, &columns, &after, test_span); + + assert!(result.is_ok()); + + let result_rec_tmp = result.unwrap(); + let result_record = result_rec_tmp.as_record().unwrap(); + + assert_eq!(*result_record.get_index(0).unwrap().0, "b".to_string()); + assert_eq!(*result_record.get_index(1).unwrap().0, "c".to_string()); + assert_eq!(*result_record.get_index(2).unwrap().0, "a".to_string()); + assert_eq!(*result_record.get_index(3).unwrap().0, "d".to_string()); + } + + #[test] + fn move_after_with_multiple_columns() { + let test_span = Span::test_data(); + let test_record = get_test_record(vec!["a", "b", "c", "d", "e"], vec![1, 2, 3, 4, 5]); + let after: Spanned = Spanned { + item: BeforeOrAfter::After("c".to_string()), + span: test_span, + }; + let columns = [Value::test_string("b"), Value::test_string("e")]; + + // corresponds to: {a: 1, b: 2, c: 3, d: 4, e: 5} | move b e --after c + let result = move_record_columns(&test_record, &columns, &after, test_span); + + assert!(result.is_ok()); + + let result_rec_tmp = result.unwrap(); + let result_record = result_rec_tmp.as_record().unwrap(); + + assert_eq!(*result_record.get_index(0).unwrap().0, "a".to_string()); + assert_eq!(*result_record.get_index(1).unwrap().0, "c".to_string()); + assert_eq!(*result_record.get_index(2).unwrap().0, "b".to_string()); + assert_eq!(*result_record.get_index(3).unwrap().0, "e".to_string()); + assert_eq!(*result_record.get_index(4).unwrap().0, "d".to_string()); + } + + #[test] + fn move_before_with_single_column() { + let test_span = Span::test_data(); + let test_record = get_test_record(vec!["a", "b", "c", "d"], vec![1, 2, 3, 4]); + let after: Spanned = Spanned { + item: BeforeOrAfter::Before("b".to_string()), + span: test_span, + }; + let columns = [Value::test_string("d")]; + + // corresponds to: {a: 1, b: 2, c: 3, d: 4} | move d --before b + let result = move_record_columns(&test_record, &columns, &after, test_span); + + assert!(result.is_ok()); + + let result_rec_tmp = result.unwrap(); + let result_record = result_rec_tmp.as_record().unwrap(); + + assert_eq!(*result_record.get_index(0).unwrap().0, "a".to_string()); + assert_eq!(*result_record.get_index(1).unwrap().0, "d".to_string()); + assert_eq!(*result_record.get_index(2).unwrap().0, "b".to_string()); + assert_eq!(*result_record.get_index(3).unwrap().0, "c".to_string()); + } + + #[test] + fn move_before_with_multiple_columns() { + let test_span = Span::test_data(); + let test_record = get_test_record(vec!["a", "b", "c", "d", "e"], vec![1, 2, 3, 4, 5]); + let after: Spanned = Spanned { + item: BeforeOrAfter::Before("b".to_string()), + span: test_span, + }; + let columns = [Value::test_string("c"), Value::test_string("e")]; + + // corresponds to: {a: 1, b: 2, c: 3, d: 4, e: 5} | move c e --before b + let result = move_record_columns(&test_record, &columns, &after, test_span); + + assert!(result.is_ok()); + + let result_rec_tmp = result.unwrap(); + let result_record = result_rec_tmp.as_record().unwrap(); + + assert_eq!(*result_record.get_index(0).unwrap().0, "a".to_string()); + assert_eq!(*result_record.get_index(1).unwrap().0, "c".to_string()); + assert_eq!(*result_record.get_index(2).unwrap().0, "e".to_string()); + assert_eq!(*result_record.get_index(3).unwrap().0, "b".to_string()); + assert_eq!(*result_record.get_index(4).unwrap().0, "d".to_string()); + } + + #[test] + fn move_with_multiple_columns_reorders_columns() { + let test_span = Span::test_data(); + let test_record = get_test_record(vec!["a", "b", "c", "d", "e"], vec![1, 2, 3, 4, 5]); + let after: Spanned = Spanned { + item: BeforeOrAfter::After("e".to_string()), + span: test_span, + }; + let columns = [ + Value::test_string("d"), + Value::test_string("c"), + Value::test_string("a"), + ]; + + // corresponds to: {a: 1, b: 2, c: 3, d: 4, e: 5} | move d c a --after e + let result = move_record_columns(&test_record, &columns, &after, test_span); + + assert!(result.is_ok()); + + let result_rec_tmp = result.unwrap(); + let result_record = result_rec_tmp.as_record().unwrap(); + + assert_eq!(*result_record.get_index(0).unwrap().0, "b".to_string()); + assert_eq!(*result_record.get_index(1).unwrap().0, "e".to_string()); + assert_eq!(*result_record.get_index(2).unwrap().0, "d".to_string()); + assert_eq!(*result_record.get_index(3).unwrap().0, "c".to_string()); + assert_eq!(*result_record.get_index(4).unwrap().0, "a".to_string()); + } + + #[test] + fn move_fails_when_pivot_not_present() { + let test_span = Span::test_data(); + let test_record = get_test_record(vec!["a", "b"], vec![1, 2]); + let after: Spanned = Spanned { + item: BeforeOrAfter::Before("non-existent".to_string()), + span: test_span, + }; + let columns = [Value::test_string("a")]; + + let result = move_record_columns(&test_record, &columns, &after, test_span); + + assert!(result.is_err()); + } + + #[test] + fn move_fails_when_column_not_present() { + let test_span = Span::test_data(); + let test_record = get_test_record(vec!["a", "b"], vec![1, 2]); + let after: Spanned = Spanned { + item: BeforeOrAfter::Before("b".to_string()), + span: test_span, + }; + let columns = [Value::test_string("a"), Value::test_string("non-existent")]; + + let result = move_record_columns(&test_record, &columns, &after, test_span); + + assert!(result.is_err()); + } + + #[test] + fn move_fails_when_column_is_also_pivot() { + let test_span = Span::test_data(); + let test_record = get_test_record(vec!["a", "b", "c", "d"], vec![1, 2, 3, 4]); + let after: Spanned = Spanned { + item: BeforeOrAfter::After("b".to_string()), + span: test_span, + }; + let columns = [Value::test_string("b"), Value::test_string("d")]; + + let result = move_record_columns(&test_record, &columns, &after, test_span); + + assert!(result.is_err()); + } }