From 442faa5576a238c3721fa335ca2fb5b1abe44740 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Thu, 28 Mar 2024 20:18:43 +0000 Subject: [PATCH] Make `Record.cols` private (#12317) # Description Makes the `cols` field in `Record` private and fixes the implementation of `rename` to account for this change. --- crates/nu-command/src/filters/rename.rs | 132 +++++++++++++----------- crates/nu-protocol/src/value/record.rs | 6 +- 2 files changed, 73 insertions(+), 65 deletions(-) diff --git a/crates/nu-command/src/filters/rename.rs b/crates/nu-command/src/filters/rename.rs index bc05153217..8d132bb99b 100644 --- a/crates/nu-command/src/filters/rename.rs +++ b/crates/nu-command/src/filters/rename.rs @@ -1,7 +1,6 @@ use indexmap::IndexMap; use nu_engine::{command_prelude::*, get_eval_block_with_early_return}; use nu_protocol::engine::Closure; -use std::collections::HashSet; #[derive(Clone)] pub struct Rename; @@ -157,72 +156,85 @@ fn rename( move |item| { let span = item.span(); match item { - Value::Record { - val: mut record, .. - } => { - if let Some((engine_state, block, mut stack, env_vars, env_hidden)) = - block_info.clone() - { - for c in &mut record.cols { - stack.with_env(&env_vars, &env_hidden); + Value::Record { val: record, .. } => { + let record = + if let Some((engine_state, block, mut stack, env_vars, env_hidden)) = + block_info.clone() + { + record + .into_iter() + .map(|(col, val)| { + stack.with_env(&env_vars, &env_hidden); - if let Some(var) = block.signature.get_positional(0) { - if let Some(var_id) = &var.var_id { - stack.add_var(*var_id, Value::string(c.clone(), span)) - } - } + if let Some(var) = block.signature.get_positional(0) { + if let Some(var_id) = &var.var_id { + stack.add_var( + *var_id, + Value::string(col.clone(), span), + ) + } + } - let eval_result = eval_block_with_early_return( - &engine_state, - &mut stack, - &block, - Value::string(c.clone(), span).into_pipeline_data(), - ); + eval_block_with_early_return( + &engine_state, + &mut stack, + &block, + Value::string(col, span).into_pipeline_data(), + ) + .and_then(|data| data.collect_string_strict(span)) + .map(|(col, _, _)| (col, val)) + }) + .collect::>() + } else { + match &specified_column { + Some(columns) => { + // record columns are unique so we can track the number + // of renamed columns to check if any were missed + let mut renamed = 0; + let record = record.into_iter().map(|(col, val)| { + let col = if let Some(col) = columns.get(&col) { + renamed += 1; + col.clone() + } else { + col + }; - match eval_result { - Err(e) => return Value::error(e, span), - Ok(res) => match res.collect_string_strict(span) { - Err(e) => return Value::error(e, span), - Ok(new_c) => *c = new_c.0, - }, - } - } - } else { - match &specified_column { - Some(c) => { - let mut column_to_rename: HashSet = HashSet::from_iter(c.keys().cloned()); - for val in record.cols.iter_mut() { - if c.contains_key(val) { - column_to_rename.remove(val); - *val = c.get(val).expect("already check exists").to_owned(); + (col, val) + }).collect::(); + + let missing_column = if renamed < columns.len() { + columns.iter().find_map(|(col, new_col)| { + (!record.contains(new_col)).then_some(col) + }) + } else { + None + }; + + if let Some(missing) = missing_column { + Err(ShellError::UnsupportedInput { + msg: format!("The column '{missing}' does not exist in the input"), + input: "value originated from here".into(), + msg_span: head_span, + input_span: span, + }) + } else { + Ok(record) } } - if !column_to_rename.is_empty() { - let not_exists_column = - column_to_rename.into_iter().next().expect( - "already checked column to rename still exists", - ); - return Value::error( - ShellError::UnsupportedInput { msg: format!( - "The column '{not_exists_column}' does not exist in the input", - ), input: "value originated from here".into(), msg_span: head_span, input_span: span }, - span, - ); - } + None => Ok(record + .into_iter() + .enumerate() + .map(|(i, (col, val))| { + (columns.get(i).cloned().unwrap_or(col), val) + }) + .collect()), } - None => { - for (idx, val) in columns.iter().enumerate() { - if idx >= record.len() { - // skip extra new columns names if we already reached the final column - break; - } - record.cols[idx] = val.clone(); - } - } - } + }; + + match record { + Ok(record) => Value::record(record, span), + Err(err) => Value::error(err, span), } - - Value::record(*record, span) } // Propagate errors by explicitly matching them before the final case. Value::Error { .. } => item.clone(), diff --git a/crates/nu-protocol/src/value/record.rs b/crates/nu-protocol/src/value/record.rs index 405d4e67f2..d1dc28973c 100644 --- a/crates/nu-protocol/src/value/record.rs +++ b/crates/nu-protocol/src/value/record.rs @@ -6,11 +6,7 @@ use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct Record { - /// Don't use this field publicly! - /// - /// Only public as command `rename` is not reimplemented in a sane way yet - /// Using it or making `vals` public will draw shaming by @sholderbach - pub cols: Vec, + cols: Vec, vals: Vec, }