From 227d1d9508737abc6ab6f63bdf3c60c1afe15040 Mon Sep 17 00:00:00 2001 From: baehyunsol <47506131+baehyunsol@users.noreply.github.com> Date: Tue, 4 Jul 2023 00:19:50 +0900 Subject: [PATCH] make the behaviours of `last` and `first` more consistent (#9582) # Description - fixes #9567 I have fixed everything mentioned in the issue, and made their help messages more similar. # User-Facing Changes - Previously, `last` on binary data returned an integer. Now it returns a binary - Now, `[] | last` and `[] | first` are both errors. - Now, `ls | table | first` and `ls | table | last` are both errors. --- crates/nu-command/src/filters/first.rs | 31 ++++- crates/nu-command/src/filters/last.rs | 146 +++++++++++++++++----- crates/nu-command/tests/commands/first.rs | 34 +++++ crates/nu-command/tests/commands/last.rs | 34 +++++ 4 files changed, 207 insertions(+), 38 deletions(-) diff --git a/crates/nu-command/src/filters/first.rs b/crates/nu-command/src/filters/first.rs index d3404f74af..153391509d 100644 --- a/crates/nu-command/src/filters/first.rs +++ b/crates/nu-command/src/filters/first.rs @@ -105,6 +105,13 @@ fn first_helper( let ctrlc = engine_state.ctrlc.clone(); let metadata = input.metadata(); + // early exit for `first 0` + if rows_desired == 0 { + return Ok(Vec::::new() + .into_pipeline_data(ctrlc) + .set_metadata(metadata)); + } + match input { PipelineData::Value(val, _) => match val { Value::List { vals, .. } => { @@ -123,11 +130,25 @@ fn first_helper( } } Value::Binary { val, span } => { - let slice: Vec = val.into_iter().take(rows_desired).collect(); - Ok(PipelineData::Value( - Value::Binary { val: slice, span }, - metadata, - )) + if return_single_element { + if val.is_empty() { + Err(ShellError::AccessEmptyContent { span: head }) + } else { + Ok(PipelineData::Value( + Value::Int { + val: val[0] as i64, + span, + }, + metadata, + )) + } + } else { + let slice: Vec = val.into_iter().take(rows_desired).collect(); + Ok(PipelineData::Value( + Value::Binary { val: slice, span }, + metadata, + )) + } } Value::Range { val, .. } => { if return_single_element { diff --git a/crates/nu-command/src/filters/last.rs b/crates/nu-command/src/filters/last.rs index 97a6688a59..c33febe9b8 100644 --- a/crates/nu-command/src/filters/last.rs +++ b/crates/nu-command/src/filters/last.rs @@ -35,6 +35,7 @@ impl Command for Last { Type::List(Box::new(Type::Any)), Type::Any, ), + (Type::Binary, Type::Binary), ]) .optional( "rows", @@ -52,7 +53,7 @@ impl Command for Last { vec![ Example { example: "[1,2,3] | last 2", - description: "Get the last 2 items", + description: "Return the last 2 items of a list/table", result: Some(Value::List { vals: vec![Value::test_int(2), Value::test_int(3)], span: Span::test_data(), @@ -60,9 +61,17 @@ impl Command for Last { }, Example { example: "[1,2,3] | last", - description: "Get the last item", + description: "Return the last item of a list/table", result: Some(Value::test_int(3)), }, + Example { + example: "0x[01 23 45] | last 2", + description: "Return the last 2 bytes of a binary value", + result: Some(Value::Binary { + val: vec![0x23, 0x45], + span: Span::test_data(), + }), + }, ] } @@ -73,45 +82,116 @@ impl Command for Last { call: &Call, input: PipelineData, ) -> Result { - let metadata = input.metadata(); - let span = call.head; - + let head = call.head; let rows: Option = call.opt(engine_state, stack, 0)?; - let to_keep = match rows.unwrap_or(1) { - 0 => { - // early exit for `last 0` - return Ok(Vec::::new() - .into_pipeline_data(engine_state.ctrlc.clone()) - .set_metadata(metadata)); - } - i if i < 0 => { - return Err(ShellError::NeedsPositiveValue(span)); - } - i => i as usize, + + // FIXME: Please read the FIXME message in `first.rs`'s `first_helper` implementation. + // It has the same issue. + let return_single_element = rows.is_none(); + let rows_desired: usize = match rows { + Some(i) if i < 0 => return Err(ShellError::NeedsPositiveValue(head)), + Some(x) => x as usize, + None => 1, }; - // only keep last `to_keep` rows in memory - let mut buf = VecDeque::<_>::new(); - for row in input.into_iter_strict(call.head)? { - if buf.len() == to_keep { - buf.pop_front(); - } + let ctrlc = engine_state.ctrlc.clone(); + let metadata = input.metadata(); - buf.push_back(row); + // early exit for `last 0` + if rows_desired == 0 { + return Ok(Vec::::new() + .into_pipeline_data(ctrlc) + .set_metadata(metadata)); } - if rows.is_some() { - Ok(buf - .into_pipeline_data(engine_state.ctrlc.clone()) - .set_metadata(metadata)) - } else { - let last = buf.pop_back(); + match input { + PipelineData::ListStream(_, _) | PipelineData::Value(Value::Range { .. }, _) => { + let iterator = input.into_iter_strict(head)?; - if let Some(last) = last { - Ok(last.into_pipeline_data().set_metadata(metadata)) - } else { - Ok(PipelineData::empty().set_metadata(metadata)) + // only keep last `rows_desired` rows in memory + let mut buf = VecDeque::<_>::new(); + + for row in iterator { + if buf.len() == rows_desired { + buf.pop_front(); + } + + buf.push_back(row); + } + + if return_single_element { + if let Some(last) = buf.pop_back() { + Ok(last.into_pipeline_data().set_metadata(metadata)) + } else { + Ok(PipelineData::empty().set_metadata(metadata)) + } + } else { + Ok(buf.into_pipeline_data(ctrlc).set_metadata(metadata)) + } } + PipelineData::Value(val, _) => match val { + Value::List { vals, .. } => { + if return_single_element { + if let Some(v) = vals.last() { + Ok(v.clone().into_pipeline_data()) + } else { + Err(ShellError::AccessEmptyContent { span: head }) + } + } else { + Ok(vals + .into_iter() + .rev() + .take(rows_desired) + .rev() + .into_pipeline_data(ctrlc) + .set_metadata(metadata)) + } + } + Value::Binary { val, span } => { + if return_single_element { + if let Some(b) = val.last() { + Ok(PipelineData::Value( + Value::Int { + val: *b as i64, + span, + }, + metadata, + )) + } else { + Err(ShellError::AccessEmptyContent { span: head }) + } + } else { + let slice: Vec = + val.into_iter().rev().take(rows_desired).rev().collect(); + Ok(PipelineData::Value( + Value::Binary { val: slice, span }, + metadata, + )) + } + } + // Propagate errors by explicitly matching them before the final case. + Value::Error { error } => Err(*error), + other => Err(ShellError::OnlySupportsThisInputType { + exp_input_type: "list, binary or range".into(), + wrong_type: other.get_type().to_string(), + dst_span: head, + src_span: other.expect_span(), + }), + }, + PipelineData::ExternalStream { span, .. } => { + Err(ShellError::OnlySupportsThisInputType { + exp_input_type: "list, binary or range".into(), + wrong_type: "raw data".into(), + dst_span: head, + src_span: span, + }) + } + PipelineData::Empty => Err(ShellError::OnlySupportsThisInputType { + exp_input_type: "list, binary or range".into(), + wrong_type: "null".into(), + dst_span: call.head, + src_span: call.head, + }), } } } diff --git a/crates/nu-command/tests/commands/first.rs b/crates/nu-command/tests/commands/first.rs index 1c3dc4ccd7..003df7c246 100644 --- a/crates/nu-command/tests/commands/first.rs +++ b/crates/nu-command/tests/commands/first.rs @@ -79,6 +79,28 @@ fn gets_first_row_as_list_when_amount_given() { assert_eq!(actual.out, "list (stream)"); } +#[test] +fn gets_first_bytes() { + let actual = nu!(pipeline( + r#" + (0x[aa bb cc] | first 2) == 0x[aa bb] + "# + )); + + assert_eq!(actual.out, "true"); +} + +#[test] +fn gets_first_byte() { + let actual = nu!(pipeline( + r#" + 0x[aa bb cc] | first + "# + )); + + assert_eq!(actual.out, "170"); +} + #[test] // covers a situation where `first` used to behave strangely on list input fn works_with_binary_list() { @@ -98,3 +120,15 @@ fn errors_on_negative_rows() { assert!(actual.err.contains("use a positive value")); } + +#[test] +fn errors_on_empty_list_when_no_rows_given() { + let actual = nu!(pipeline( + r#" + [] + | first + "# + )); + + assert!(actual.err.contains("index too large")); +} diff --git a/crates/nu-command/tests/commands/last.rs b/crates/nu-command/tests/commands/last.rs index af0029dba5..159760b348 100644 --- a/crates/nu-command/tests/commands/last.rs +++ b/crates/nu-command/tests/commands/last.rs @@ -79,6 +79,28 @@ fn gets_last_row_as_list_when_amount_given() { assert_eq!(actual.out, "list (stream)"); } +#[test] +fn gets_last_bytes() { + let actual = nu!(pipeline( + r#" + (0x[aa bb cc] | last 2) == 0x[bb cc] + "# + )); + + assert_eq!(actual.out, "true"); +} + +#[test] +fn gets_last_byte() { + let actual = nu!(pipeline( + r#" + 0x[aa bb cc] | last + "# + )); + + assert_eq!(actual.out, "204"); +} + #[test] fn last_errors_on_negative_index() { let actual = nu!(pipeline( @@ -97,3 +119,15 @@ fn fail_on_non_iterator() { assert!(actual.err.contains("only_supports_this_input_type")); } + +#[test] +fn errors_on_empty_list_when_no_rows_given() { + let actual = nu!(pipeline( + r#" + [] + | last + "# + )); + + assert!(actual.err.contains("index too large")); +}