From 56aacc485256059ca64d367dfa42d90ea9daef73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=BD=C3=A1dn=C3=ADk?= Date: Sun, 20 Feb 2022 16:27:59 +0200 Subject: [PATCH] Use environment variable for env_conversions (#4566) * Handle string->value env conv. with env. var. Also adds the environment variable for Path/PATH and removes it from config. * Simplify getting the string->value conversion * Refactor env conversion into its own function * Use env var for to_string conversion; Remove conf * Fix indentation in default config --- crates/nu-engine/src/env.rs | 167 +++++++++++++++++-------------- crates/nu-protocol/src/config.rs | 56 +---------- src/commands.rs | 2 +- src/default_config.nu | 32 +++--- src/eval_file.rs | 12 +-- src/repl.rs | 13 +-- 6 files changed, 116 insertions(+), 166 deletions(-) diff --git a/crates/nu-engine/src/env.rs b/crates/nu-engine/src/env.rs index 39faad40ed..07da45414e 100644 --- a/crates/nu-engine/src/env.rs +++ b/crates/nu-engine/src/env.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; +use nu_protocol::ast::PathMember; use nu_protocol::engine::{EngineState, Stack}; use nu_protocol::{Config, PipelineData, ShellError, Value}; @@ -11,63 +12,36 @@ const ENV_SEP: &str = ";"; #[cfg(not(windows))] const ENV_SEP: &str = ":"; +const ENV_CONVERSIONS: &str = "ENV_CONVERSIONS"; + +enum ConversionResult { + Ok(Value), + ConversionError(ShellError), // Failure during the conversion itself + GeneralError(ShellError), // Other error not directly connected to running the conversion + CellPathError, // Error looking up the ENV_VAR.to_/from_string fields in $env.ENV_CONVERSIONS +} + /// Translate environment variables from Strings to Values. Requires config to be already set up in /// case the user defined custom env conversions in config.nu. /// /// It returns Option instead of Result since we do want to translate all the values we can and /// skip errors. This function is called in the main() so we want to keep running, we cannot just /// exit. -pub fn convert_env_values( - engine_state: &mut EngineState, - stack: &Stack, - config: &Config, -) -> Option { +pub fn convert_env_values(engine_state: &mut EngineState, stack: &Stack) -> Option { let mut error = None; let mut new_scope = HashMap::new(); for (name, val) in &engine_state.env_vars { - if let Some(env_conv) = config.env_conversions.get(name) { - if let Some((block_id, from_span)) = env_conv.from_string { - let val_span = match val.span() { - Ok(sp) => sp, - Err(e) => { - error = error.or(Some(e)); - continue; - } - }; - - let block = engine_state.get_block(block_id); - - if let Some(var) = block.signature.get_positional(0) { - let mut stack = stack.gather_captures(&block.captures); - if let Some(var_id) = &var.var_id { - stack.add_var(*var_id, val.clone()); - } - - let result = - eval_block(engine_state, &mut stack, block, PipelineData::new(val_span)); - - match result { - Ok(data) => { - let val = data.into_value(val_span); - new_scope.insert(name.to_string(), val); - } - Err(e) => error = error.or(Some(e)), - } - } else { - error = error.or_else(|| { - Some(ShellError::MissingParameter( - "block input".into(), - from_span, - )) - }); - } - } else { - new_scope.insert(name.to_string(), val.clone()); + match get_converted_value(engine_state, stack, name, val, "from_string") { + ConversionResult::Ok(v) => { + let _ = new_scope.insert(name.to_string(), v); + } + ConversionResult::ConversionError(e) => error = error.or(Some(e)), + ConversionResult::GeneralError(_) => continue, + ConversionResult::CellPathError => { + let _ = new_scope.insert(name.to_string(), val.clone()); } - } else { - new_scope.insert(name.to_string(), val.clone()); } } @@ -86,36 +60,11 @@ pub fn env_to_string( stack: &Stack, config: &Config, ) -> Result { - if let Some(env_conv) = config.env_conversions.get(env_name) { - if let Some((block_id, to_span)) = env_conv.to_string { - let block = engine_state.get_block(block_id); - - if let Some(var) = block.signature.get_positional(0) { - let val_span = value.span()?; - let mut stack = stack.gather_captures(&block.captures); - - if let Some(var_id) = &var.var_id { - stack.add_var(*var_id, value); - } - - Ok( - // This one is OK to fail: We want to know if custom conversion is working - eval_block(engine_state, &mut stack, block, PipelineData::new(val_span))? - .into_value(val_span) - .as_string()?, - ) - } else { - Err(ShellError::MissingParameter("block input".into(), to_span)) - } - } else { - // Do not fail here. Must succeed, otherwise setting a non-string env var would constantly - // throw errors when running externals etc. - Ok(value.into_string(ENV_SEP, config)) - } - } else { - // Do not fail here. Must succeed, otherwise setting a non-string env var would constantly - // throw errors when running externals etc. - Ok(value.into_string(ENV_SEP, config)) + match get_converted_value(engine_state, stack, env_name, &value, "to_string") { + ConversionResult::Ok(v) => Ok(v.as_string()?), + ConversionResult::ConversionError(e) => Err(e), + ConversionResult::GeneralError(e) => Err(e), + ConversionResult::CellPathError => Ok(value.into_string(ENV_SEP, config)), } } @@ -165,3 +114,71 @@ pub fn current_dir_str(engine_state: &EngineState, stack: &Stack) -> Result Result { current_dir_str(engine_state, stack).map(PathBuf::from) } + +fn get_converted_value( + engine_state: &EngineState, + stack: &Stack, + name: &str, + orig_val: &Value, + direction: &str, +) -> ConversionResult { + if let Some(env_conversions) = stack.get_env_var(engine_state, ENV_CONVERSIONS) { + let env_span = match env_conversions.span() { + Ok(span) => span, + Err(e) => { + return ConversionResult::GeneralError(e); + } + }; + let val_span = match orig_val.span() { + Ok(span) => span, + Err(e) => { + return ConversionResult::GeneralError(e); + } + }; + + let path_members = &[ + PathMember::String { + val: name.to_string(), + span: env_span, + }, + PathMember::String { + val: direction.to_string(), + span: env_span, + }, + ]; + + if let Ok(Value::Block { + val: block_id, + span: from_span, + .. + }) = env_conversions.follow_cell_path(path_members) + { + let block = engine_state.get_block(block_id); + + if let Some(var) = block.signature.get_positional(0) { + let mut stack = stack.gather_captures(&block.captures); + if let Some(var_id) = &var.var_id { + stack.add_var(*var_id, orig_val.clone()); + } + + let result = + eval_block(engine_state, &mut stack, block, PipelineData::new(val_span)); + + match result { + Ok(data) => ConversionResult::Ok(data.into_value(val_span)), + Err(e) => ConversionResult::ConversionError(e), + } + } else { + // This one is OK to fail: We want to know if custom conversion is working + ConversionResult::ConversionError(ShellError::MissingParameter( + "block input".into(), + from_span, + )) + } + } else { + ConversionResult::CellPathError + } + } else { + ConversionResult::CellPathError + } +} diff --git a/crates/nu-protocol/src/config.rs b/crates/nu-protocol/src/config.rs index 0e64dc4afc..effc1a3c29 100644 --- a/crates/nu-protocol/src/config.rs +++ b/crates/nu-protocol/src/config.rs @@ -1,43 +1,9 @@ -use crate::{BlockId, ShellError, Span, Value}; +use crate::{ShellError, Span, Value}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; const ANIMATE_PROMPT_DEFAULT: bool = true; -#[derive(Serialize, Deserialize, Clone, Debug)] -pub struct EnvConversion { - pub from_string: Option<(BlockId, Span)>, - pub to_string: Option<(BlockId, Span)>, -} - -impl EnvConversion { - pub fn from_record(value: &Value) -> Result { - let record = value.as_record()?; - - let mut conv_map = HashMap::new(); - - for (k, v) in record.0.iter().zip(record.1) { - if (k == "from_string") || (k == "to_string") { - conv_map.insert(k.as_str(), (v.as_block()?, v.span()?)); - } else { - return Err(ShellError::UnsupportedConfigValue( - "'from_string' and 'to_string' fields".into(), - k.into(), - value.span()?, - )); - } - } - - let from_string = conv_map.get("from_string").cloned(); - let to_string = conv_map.get("to_string").cloned(); - - Ok(EnvConversion { - from_string, - to_string, - }) - } -} - /// Definition of a parsed keybinding from the config object #[derive(Serialize, Deserialize, Clone, Debug)] pub struct ParsedKeybinding { @@ -60,7 +26,6 @@ pub struct Config { pub filesize_format: String, pub use_ansi_coloring: bool, pub quick_completions: bool, - pub env_conversions: HashMap, pub edit_mode: String, pub max_history_size: i64, pub log_level: String, @@ -84,7 +49,6 @@ impl Default for Config { filesize_format: "auto".into(), use_ansi_coloring: true, quick_completions: false, - env_conversions: HashMap::new(), // TODO: Add default conversoins edit_mode: "emacs".into(), max_history_size: 1000, log_level: String::new(), @@ -210,24 +174,6 @@ impl Value { eprintln!("$config.filesize_format is not a string") } } - "env_conversions" => { - if let Ok((env_vars, conversions)) = value.as_record() { - let mut env_conversions = HashMap::new(); - - for (env_var, record) in env_vars.iter().zip(conversions) { - // println!("{}: {:?}", env_var, record); - if let Ok(conversion) = EnvConversion::from_record(record) { - env_conversions.insert(env_var.into(), conversion); - } else { - eprintln!("$config.env_conversions has incorrect conversion") - } - } - - config.env_conversions = env_conversions; - } else { - eprintln!("$config.env_conversions is not a record") - } - } "edit_mode" => { if let Ok(v) = value.as_string() { config.edit_mode = v.to_lowercase(); diff --git a/src/commands.rs b/src/commands.rs index 474afe3d6e..9652f87432 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -89,7 +89,7 @@ pub(crate) fn evaluate( } // Translate environment variables from Strings to Values - if let Some(e) = convert_env_values(engine_state, &stack, &config) { + if let Some(e) = convert_env_values(engine_state, &stack) { let working_set = StateWorkingSet::new(engine_state); report_error(&working_set, &e); std::process::exit(1); diff --git a/src/default_config.nu b/src/default_config.nu index 2223af0027..b394d1c03d 100644 --- a/src/default_config.nu +++ b/src/default_config.nu @@ -25,6 +25,20 @@ let-env PROMPT_INDICATOR_VI_INSERT = ": " let-env PROMPT_INDICATOR_VI_NORMAL = "〉" let-env PROMPT_MULTILINE_INDICATOR = "::: " +# Specifies how environment variables are: +# - converted from a string to a value on Nushell startup (from_string) +# - converted from a value back to a string when running extrnal commands (to_string) +let-env ENV_CONVERSIONS = { + "PATH": { + from_string: { |s| $s | split row (char esep) } + to_string: { |v| $v | str collect (char esep) } + } + "Path": { + from_string: { |s| $s | split row (char esep) } + to_string: { |v| $v | str collect (char esep) } + } +} + # Custom completions for external commands (those outside of Nushell) # Each completions has two parts: the form of the external command, including its flags and parameters # and a helper command that knows how to complete values for those flags and parameters @@ -93,7 +107,7 @@ extern "git push" [ --ipv6(-6) # use IPv6 addresses only ] -# The default config record. This is where much of your global configuration is setup. +# The default config record. This is where much of your global configuration is setup. let $config = { filesize_metric: $false table_mode: rounded # basic, compact, compact_double, light, thin, with_love, rounded, reinforced, heavy, none, other @@ -124,12 +138,6 @@ let $config = { float_precision: 2 use_ansi_coloring: $true filesize_format: "b" # b, kb, kib, mb, mib, gb, gib, tb, tib, pb, pib, eb, eib, zb, zib, auto - env_conversions: { - "PATH": { - from_string: { |s| $s | split row (char esep) } - to_string: { |v| $v | str collect (char esep) } - } - } edit_mode: emacs # emacs, vi max_history_size: 10000 menu_config: { @@ -141,11 +149,11 @@ let $config = { marker: "| " } history_config: { - page_size: 10 - selector: ":" - text_style: green - selected_text_style: green_reverse - marker: "? " + page_size: 10 + selector: ":" + text_style: green + selected_text_style: green_reverse + marker: "? " } keybindings: [ { diff --git a/src/eval_file.rs b/src/eval_file.rs index 9ed28b66e0..17d387dd8d 100644 --- a/src/eval_file.rs +++ b/src/eval_file.rs @@ -34,18 +34,8 @@ pub(crate) fn evaluate( }, ); - let config = match stack.get_config() { - Ok(config) => config, - Err(e) => { - let working_set = StateWorkingSet::new(engine_state); - - report_error(&working_set, &e); - Config::default() - } - }; - // Translate environment variables from Strings to Values - if let Some(e) = convert_env_values(engine_state, &stack, &config) { + if let Some(e) = convert_env_values(engine_state, &stack) { let working_set = StateWorkingSet::new(engine_state); report_error(&working_set, &e); std::process::exit(1); diff --git a/src/repl.rs b/src/repl.rs index 084c501268..ce0e05f140 100644 --- a/src/repl.rs +++ b/src/repl.rs @@ -59,17 +59,6 @@ pub(crate) fn evaluate( config_files::read_config_file(engine_state, &mut stack, config_file); let history_path = config_files::create_history_path(); - // Load config struct form config variable - let config = match stack.get_config() { - Ok(config) => config, - Err(e) => { - let working_set = StateWorkingSet::new(engine_state); - - report_error(&working_set, &e); - Config::default() - } - }; - // logger(|builder| { // configure(&config.log_level, builder)?; // // trace_filters(self, builder)?; @@ -88,7 +77,7 @@ pub(crate) fn evaluate( } // Translate environment variables from Strings to Values - if let Some(e) = convert_env_values(engine_state, &stack, &config) { + if let Some(e) = convert_env_values(engine_state, &stack) { let working_set = StateWorkingSet::new(engine_state); report_error(&working_set, &e); }