columns now errors when given a non-record non-table (#7593)

# Description

This is for consistency with the new `values` command. Previously it
would return a completely empty record (??!) when given an
incorrectly-typed value.

# User-Facing Changes

See title.

# 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

# 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:
Leon 2022-12-24 23:08:25 +10:00 committed by GitHub
parent 11bdab7e61
commit e76b38882c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 37 additions and 14 deletions

View File

@ -2,8 +2,8 @@ use nu_engine::column::get_columns;
use nu_protocol::ast::Call; use nu_protocol::ast::Call;
use nu_protocol::engine::{Command, EngineState, Stack}; use nu_protocol::engine::{Command, EngineState, Stack};
use nu_protocol::{ use nu_protocol::{
Category, Example, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData, ShellError, Category, Example, IntoInterruptiblePipelineData, PipelineData, ShellError, Signature, Span,
Signature, Span, Type, Value, Type, Value,
}; };
#[derive(Clone)] #[derive(Clone)]
@ -80,9 +80,11 @@ impl Command for Columns {
fn getcol( fn getcol(
engine_state: &EngineState, engine_state: &EngineState,
span: Span, head: Span,
input: PipelineData, input: PipelineData,
) -> Result<PipelineData, ShellError> { ) -> Result<PipelineData, ShellError> {
let ctrlc = engine_state.ctrlc.clone();
let metadata = input.metadata();
match input { match input {
PipelineData::Empty => Ok(PipelineData::Empty), PipelineData::Empty => Ok(PipelineData::Empty),
PipelineData::Value( PipelineData::Value(
@ -96,7 +98,8 @@ fn getcol(
Ok(input_cols Ok(input_cols
.into_iter() .into_iter()
.map(move |x| Value::String { val: x, span }) .map(move |x| Value::String { val: x, span })
.into_pipeline_data(engine_state.ctrlc.clone())) .into_pipeline_data(ctrlc)
.set_metadata(metadata))
} }
PipelineData::Value(Value::CustomValue { val, span }, ..) => { PipelineData::Value(Value::CustomValue { val, span }, ..) => {
// TODO: should we get CustomValue to expose columns in a more efficient way? // TODO: should we get CustomValue to expose columns in a more efficient way?
@ -106,7 +109,8 @@ fn getcol(
Ok(input_cols Ok(input_cols
.into_iter() .into_iter()
.map(move |x| Value::String { val: x, span }) .map(move |x| Value::String { val: x, span })
.into_pipeline_data(engine_state.ctrlc.clone())) .into_pipeline_data(ctrlc)
.set_metadata(metadata))
} }
PipelineData::ListStream(stream, ..) => { PipelineData::ListStream(stream, ..) => {
let v: Vec<_> = stream.into_iter().collect(); let v: Vec<_> = stream.into_iter().collect();
@ -114,17 +118,36 @@ fn getcol(
Ok(input_cols Ok(input_cols
.into_iter() .into_iter()
.map(move |x| Value::String { val: x, span }) .map(move |x| Value::String { val: x, span: head })
.into_pipeline_data(engine_state.ctrlc.clone())) .into_pipeline_data(ctrlc)
.set_metadata(metadata))
} }
PipelineData::Value(Value::Record { cols, .. }, ..) => Ok(cols PipelineData::Value(Value::Record { cols, .. }, ..) => Ok(cols
.into_iter() .into_iter()
.map(move |x| Value::String { val: x, span }) .map(move |x| Value::String { val: x, span: head })
.into_pipeline_data(engine_state.ctrlc.clone())), .into_pipeline_data(ctrlc)
PipelineData::Value(..) | PipelineData::ExternalStream { .. } => { .set_metadata(metadata)),
let cols = vec![]; // Propagate errors
let vals = vec![]; PipelineData::Value(Value::Error { error }, ..) => Err(error),
Ok(Value::Record { cols, vals, span }.into_pipeline_data()) PipelineData::Value(other, ..) => {
Err(ShellError::OnlySupportsThisInputType(
"record or table".into(),
other.get_type().to_string(),
head,
// This line requires the Value::Error match above.
other.expect_span(),
))
}
PipelineData::ExternalStream { .. } => {
Err(ShellError::OnlySupportsThisInputType(
"record or table".into(),
"raw data".into(),
head,
// This line requires the PipelineData::Empty and PipelineData::ListStream matches above.
input
.span()
.expect("PipelineData::ExternalStream had no span"),
))
} }
} }
} }

View File

@ -176,7 +176,7 @@ fn values(
Ok(vals.into_pipeline_data(ctrlc).set_metadata(metadata)) Ok(vals.into_pipeline_data(ctrlc).set_metadata(metadata))
} }
// Propagate errors // Propagate errors
PipelineData::Value(Value::Error { error: _ }, ..) => Ok(input), PipelineData::Value(Value::Error { error }, ..) => Err(error),
PipelineData::Value(other, ..) => { PipelineData::Value(other, ..) => {
Err(ShellError::OnlySupportsThisInputType( Err(ShellError::OnlySupportsThisInputType(
"record or table".into(), "record or table".into(),