From ce78817f4113e0a11b164266da5e7ca911fd048e Mon Sep 17 00:00:00 2001 From: Leon Date: Sat, 10 Dec 2022 23:34:46 +1000 Subject: [PATCH] `$env.config` now always holds a record with only valid values (#7309) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description Closes #7059. Rather than generate a new Record each time $env.config is accessed (as described in that issue), instead `$env.config = ` now A) parses the input record, then B) un-parses it into a clean Record with only the valid values, and stores that as an env-var. The reasoning for this is that I believe `config_to_nu_record()` (the method that performs step B) will be useful in later PRs. (See below) As a result, this also "fixes" the following "bug": ``` 〉$env.config = 'butts' $env.config is not a record 〉$env.config butts ``` ~~Instead, `$env.config = 'butts'` now turns `$env.config` into the default (not the default config.nu, but `Config::default()`, which notably has empty keybindings, color_config, menus and hooks vecs).~~ This doesn't attempt to fix #7110. cc @Kangaxx-0 # Example of new behaviour OLD: ``` 〉$env.config = ($env.config | merge { foo: 1 }) $env.config.foo is an unknown config setting 〉$env.config.foo 1 ``` NEW: ``` 〉$env.config = ($env.config | merge { foo: 1 }) Error: × Config record contains invalid values or unknown settings Error: × Error while applying config changes ╭─[entry #1:1:1] 1 │ $env.config = ($env.config | merge { foo: 1 }) · ┬ · ╰── $env.config.foo is an unknown config setting ╰──── help: This value has been removed from your $env.config record. 〉$env.config.foo Error: nu::shell::column_not_found (link) × Cannot find column ╭─[entry #1:1:1] 1 │ $env.config = ($env.config | merge { foo: 1 }) · ──┬── · ╰── value originates here ╰──── ╭─[entry #2:1:1] 1 │ $env.config.foo · ─┬─ · ╰── cannot find column 'foo' ╰──── ``` # Example of new errors OLD: ``` $env.config.cd.baz is an unknown config setting $env.config.foo is an unknown config setting $env.config.bar is an unknown config setting $env.config.table.qux is an unknown config setting $env.config.history.qux is an unknown config setting ``` NEW: ``` Error: × Config record contains invalid values or unknown settings Error: × Error while applying config changes ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:267:1] 267 │ abbreviations: true # allows `cd s/o/f` to expand to `cd some/other/folder` 268 │ baz: 3, · ┬ · ╰── $env.config.cd.baz is an unknown config setting 269 │ } ╰──── help: This value has been removed from your $env.config record. Error: × Error while applying config changes ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:269:1] 269 │ } 270 │ foo: 1, · ┬ · ╰── $env.config.foo is an unknown config setting 271 │ bar: 2, ╰──── help: This value has been removed from your $env.config record. Error: × Error while applying config changes ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:270:1] 270 │ foo: 1, 271 │ bar: 2, · ┬ · ╰── $env.config.bar is an unknown config setting ╰──── help: This value has been removed from your $env.config record. Error: × Error while applying config changes ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:279:1] 279 │ } 280 │ qux: 4, · ┬ · ╰── $env.config.table.qux is an unknown config setting 281 │ } ╰──── help: This value has been removed from your $env.config record. Error: × Error while applying config changes ╭─[C:\Users\Leon\AppData\Roaming\nushell\config.nu:285:1] 285 │ file_format: "plaintext" # "sqlite" or "plaintext" 286 │ qux: 2 · ┬ · ╰── $env.config.history.qux is an unknown config setting 287 │ } ╰──── help: This value has been removed from your $env.config record. ``` # User-Facing Changes See above. # 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. --- Cargo.lock | 2 + crates/nu-protocol/Cargo.toml | 2 + crates/nu-protocol/src/config.rs | 1003 ++++++++++++----- crates/nu-protocol/src/engine/engine_state.rs | 14 +- crates/nu-protocol/src/value/mod.rs | 18 + crates/nu-protocol/tests/into_config.rs | 101 ++ 6 files changed, 827 insertions(+), 313 deletions(-) create mode 100644 crates/nu-protocol/tests/into_config.rs diff --git a/Cargo.lock b/Cargo.lock index fd26c2e95d..f6788c4b05 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2776,6 +2776,8 @@ dependencies = [ "indexmap", "miette", "nu-json", + "nu-path", + "nu-test-support", "nu-utils", "num-format", "serde", diff --git a/crates/nu-protocol/Cargo.toml b/crates/nu-protocol/Cargo.toml index f9aaca66f1..79473d818e 100644 --- a/crates/nu-protocol/Cargo.toml +++ b/crates/nu-protocol/Cargo.toml @@ -12,6 +12,8 @@ version = "0.72.2" [dependencies] nu-utils = { path = "../nu-utils", version = "0.72.2" } nu-json = { path = "../nu-json", version = "0.72.2" } +nu-path = { path = "../nu-path", version = "0.72.2" } # Used for test support +nu-test-support = { path = "../nu-test-support", version = "0.72.2" } byte-unit = "4.0.9" chrono = { version="0.4.23", features= ["serde", "std"], default-features = false } diff --git a/crates/nu-protocol/src/config.rs b/crates/nu-protocol/src/config.rs index 2dae94e8cb..2d3d798972 100644 --- a/crates/nu-protocol/src/config.rs +++ b/crates/nu-protocol/src/config.rs @@ -191,213 +191,357 @@ pub struct ExploreConfig { } impl Value { - pub fn into_config(self) -> Result { - let v = self.as_record(); - - let mut config = Config::default(); + pub fn into_config(&mut self, config: &Config) -> (Config, Option) { + // Clone the passed-in config rather than mutating it. + let mut config = config.clone(); let mut legacy_options_used = false; - if let Ok(v) = v { - for (key, value) in v.0.iter().zip(v.1) { - let key = key.as_str(); + // Vec for storing errors. + // Current Nushell behaviour (Dec 2022) is that having some typo like "always_trash": tru in your config.nu's + // set-env config record shouldn't abort all config parsing there and then. + // Thus, errors are simply collected one-by-one and wrapped in a GenericError at the end. + let mut errors = vec![]; + + // When an unsupported config value is found, ignore it. + macro_rules! invalid { + ($span:expr, $msg:literal) => { + errors.push(ShellError::GenericError( + "Error while applying config changes".into(), + format!($msg), + $span, + Some("This value will be ignored.".into()), + vec![], + )); + }; + } + // Some extra helpers + macro_rules! try_bool { + ($cols:ident, $vals:ident, $index:ident, $span:expr, $setting:ident) => { + if let Ok(b) = &$vals[$index].as_bool() { + config.$setting = *b; + } else { + invalid!(Some(*$span), "should be a bool"); + // Reconstruct + $vals[$index] = Value::boolean(config.$setting, *$span); + } + }; + } + macro_rules! try_int { + ($cols:ident, $vals:ident, $index:ident, $span:expr, $setting:ident) => { + if let Ok(b) = &$vals[$index].as_integer() { + config.$setting = *b; + } else { + invalid!(Some(*$span), "should be an int"); + // Reconstruct + $vals[$index] = Value::int(config.$setting, *$span); + } + }; + } + // When an unsupported config value is found, remove it from this record. + macro_rules! invalid_key { + // Because Value::Record discards all of the spans of its + // column names (by storing them as Strings), the key name cannot be provided + // as a value, even in key errors. + ($cols:ident, $vals:ident, $index:ident, $span:expr, $msg:literal) => { + errors.push(ShellError::GenericError( + "Error while applying config changes".into(), + format!($msg), + $span, + Some("This value will not appear in your $env.config record.".into()), + vec![], + )); + $cols.remove($index); + $vals.remove($index); + }; + } + + // Config record (self) mutation rules: + // * When parsing a config Record, if a config key error occurs, remove the key. + // * When parsing a config Record, if a config value error occurs, replace the value + // with a reconstructed Nu value for the current (unaltered) configuration for that setting. + // For instance: + // $env.config.ls.use_ls_colors = 2 results in an error, so + // the current use_ls_colors config setting is converted to a Value::Boolean and inserted in the + // record in place of the 2. + if let Value::Record { cols, vals, span } = self { + // Because this whole algorithm removes while iterating, this must iterate in reverse. + for index in (0..cols.len()).rev() { + let value = &vals[index]; + let key = cols[index].as_str(); match key { // Grouped options "ls" => { - if let Ok((cols, inner_vals)) = value.as_record() { - for (key2, value) in cols.iter().zip(inner_vals) { - let key2 = key2.as_str(); + if let Value::Record { cols, vals, span } = &mut vals[index] { + for index in (0..cols.len()).rev() { + let value = &vals[index]; + let key2 = cols[index].as_str(); match key2 { "use_ls_colors" => { - if let Ok(b) = value.as_bool() { - config.use_ls_colors = b; - } else { - eprintln!("$env.config.{}.{} is not a bool", key, key2) - } - } - "clickable_links" => { - if let Ok(b) = value.as_bool() { - config.show_clickable_links_in_ls = b; - } else { - eprintln!("$env.config.{}.{} is not a bool", key, key2) - } + try_bool!(cols, vals, index, span, use_ls_colors) } + "clickable_links" => try_bool!( + cols, + vals, + index, + span, + show_clickable_links_in_ls + ), x => { - eprintln!( - "$env.config.{}.{} is an unknown config setting", - key, x - ) + invalid_key!( + cols, + vals, + index, + value.span().ok(), + "$env.config.{key}.{x} is an unknown config setting" + ); } } } } else { - eprintln!("$env.config.{} is not a record", key); + invalid!(vals[index].span().ok(), "should be a record"); + // Reconstruct + vals[index] = Value::record( + vec!["use_ls_colors".into(), "clickable_links".into()], + vec![ + Value::boolean(config.use_ls_colors, *span), + Value::boolean(config.show_clickable_links_in_ls, *span), + ], + *span, + ); } } "cd" => { - if let Ok((cols, inner_vals)) = value.as_record() { - for (key2, value) in cols.iter().zip(inner_vals) { - let key2 = key2.as_str(); + if let Value::Record { cols, vals, span } = &mut vals[index] { + for index in (0..cols.len()).rev() { + let value = &vals[index]; + let key2 = cols[index].as_str(); match key2 { "abbreviations" => { - if let Ok(b) = value.as_bool() { - config.cd_with_abbreviations = b; - } else { - eprintln!("$env.config.{}.{} is not a bool", key, key2) - } + try_bool!(cols, vals, index, span, cd_with_abbreviations) } x => { - eprintln!( - "$env.config.{}.{} is an unknown config setting", - key, x - ) + invalid_key!( + cols, + vals, + index, + value.span().ok(), + "$env.config.{key}.{x} is an unknown config setting" + ); } } } } else { - eprintln!("$env.config.{} is not a record", key); + invalid!(vals[index].span().ok(), "should be a record"); + // Reconstruct + vals[index] = Value::record( + vec!["use_ls_colors".into(), "clickable_links".into()], + vec![ + Value::boolean(config.use_ls_colors, *span), + Value::boolean(config.show_clickable_links_in_ls, *span), + ], + *span, + ); } } "rm" => { - if let Ok((cols, inner_vals)) = value.as_record() { - for (key2, value) in cols.iter().zip(inner_vals) { - let key2 = key2.as_str(); + if let Value::Record { cols, vals, span } = &mut vals[index] { + for index in (0..cols.len()).rev() { + let value = &vals[index]; + let key2 = cols[index].as_str(); match key2 { "always_trash" => { - if let Ok(b) = value.as_bool() { - config.rm_always_trash = b; - } else { - eprintln!("$env.config.{}.{} is not a bool", key, key2) - } + try_bool!(cols, vals, index, span, rm_always_trash) } x => { - eprintln!( - "$env.config.{}.{} is an unknown config setting", - key, x - ) + invalid_key!( + cols, + vals, + index, + value.span().ok(), + "$env.config.{key}.{x} is an unknown config setting" + ); } } } } else { - eprintln!("$env.config.{} is not a record", key); + invalid!(vals[index].span().ok(), "should be a record"); + // Reconstruct + vals[index] = Value::record( + vec!["always_trash".into()], + vec![Value::boolean(config.rm_always_trash, *span)], + *span, + ); } } "history" => { - if let Ok((cols, inner_vals)) = value.as_record() { - for (key2, value) in cols.iter().zip(inner_vals) { - let key2 = key2.as_str(); + macro_rules! reconstruct_history_file_format { + ($span:expr) => { + Value::string( + match config.history_file_format { + HistoryFileFormat::Sqlite => "sqlite", + HistoryFileFormat::PlainText => "plaintext", + }, + *$span, + ) + }; + } + if let Value::Record { cols, vals, span } = &mut vals[index] { + for index in (0..cols.len()).rev() { + let value = &vals[index]; + let key2 = cols[index].as_str(); match key2 { "sync_on_enter" => { - if let Ok(b) = value.as_bool() { - config.sync_history_on_enter = b; - } else { - eprintln!("$env.config.{}.{} is not a bool", key, key2) - } + try_bool!(cols, vals, index, span, sync_history_on_enter) } "max_size" => { - if let Ok(i) = value.as_i64() { - config.max_history_size = i; - } else { - eprintln!( - "$env.config.{}.{} is not an integer", - key, key2 - ) - } + try_int!(cols, vals, index, span, max_history_size) } "file_format" => { if let Ok(v) = value.as_string() { let val_str = v.to_lowercase(); - config.history_file_format = match val_str.as_ref() { - "sqlite" => HistoryFileFormat::Sqlite, - "plaintext" => HistoryFileFormat::PlainText, + match val_str.as_ref() { + "sqlite" => { + config.history_file_format = + HistoryFileFormat::Sqlite + } + "plaintext" => { + config.history_file_format = + HistoryFileFormat::PlainText + } _ => { - eprintln!( - "unrecognized $config.{}.{} '{val_str}'; expected either 'sqlite' or 'plaintext'", - key, key2, + invalid!(Some(*span), + "unrecognized $env.config.{key}.{key2} '{val_str}'; expected either 'sqlite' or 'plaintext'" ); - HistoryFileFormat::PlainText + // Reconstruct + vals[index] = + reconstruct_history_file_format!(span); } }; } else { - eprintln!( - "$env.config.{}.{} is not a string", - key, key2 - ) + invalid!(Some(*span), "should be a string"); + // Reconstruct + vals[index] = reconstruct_history_file_format!(span); } } x => { - eprintln!( - "$env.config.{}.{} is an unknown config setting", - key, x - ) + invalid_key!( + cols, + vals, + index, + value.span().ok(), + "$env.config.{key}.{x} is an unknown config setting" + ); } } } } else { - eprintln!("$env.config.{} is not a record", key) + invalid!(vals[index].span().ok(), "should be a record"); + // Reconstruct + vals[index] = Value::record( + vec![ + "sync_on_enter".into(), + "max_size".into(), + "file_format".into(), + ], + vec![ + Value::boolean(config.sync_history_on_enter, *span), + Value::int(config.max_history_size, *span), + reconstruct_history_file_format!(span), + ], + *span, + ); } } "completions" => { - if let Ok((cols, inner_vals)) = value.as_record() { - for (key2, value) in cols.iter().zip(inner_vals) { - let key2 = key2.as_str(); + macro_rules! reconstruct_external_completer { + ($span: expr) => { + if let Some(block) = config.external_completer { + Value::Block { + val: block, + span: *$span, + } + } else { + Value::Nothing { span: *$span } + } + }; + } + macro_rules! reconstruct_external { + ($span: expr) => { + Value::record( + vec!["max_results".into(), "completer".into(), "enable".into()], + vec![ + Value::int(config.max_external_completion_results, *$span), + reconstruct_external_completer!($span), + Value::boolean(config.enable_external_completion, *$span), + ], + *$span, + ) + }; + } + if let Value::Record { cols, vals, span } = &mut vals[index] { + for index in (0..cols.len()).rev() { + let value = &vals[index]; + let key2 = cols[index].as_str(); match key2 { "quick" => { - if let Ok(b) = value.as_bool() { - config.quick_completions = b; - } else { - eprintln!("$env.config.{}.{} is not a bool", key, key2) - } + try_bool!(cols, vals, index, span, quick_completions) } "partial" => { - if let Ok(b) = value.as_bool() { - config.partial_completions = b; - } else { - eprintln!("$env.config.{}.{} is not a bool", key, key2) - } + try_bool!(cols, vals, index, span, partial_completions) } "algorithm" => { if let Ok(v) = value.as_string() { let val_str = v.to_lowercase(); - config.completion_algorithm = match val_str.as_ref() { + match val_str.as_ref() { // This should match the MatchAlgorithm enum in completions::completion_options - "prefix" => val_str, - "fuzzy" => val_str, + "prefix" | "fuzzy" => { + config.completion_algorithm = val_str + } _ => { - eprintln!( - "unrecognized $config.{}.{} '{val_str}'; expected either 'prefix' or 'fuzzy'", - key, key2 + invalid!( Some(*span), + "unrecognized $env.config.{key}.{key2} '{val_str}'; expected either 'prefix' or 'fuzzy'" + ); + // Reconstruct + vals[index] = Value::string( + config.completion_algorithm.clone(), + *span, ); - String::from("prefix") } }; } else { - eprintln!( - "$env.config.{}.{} is not a string", - key, key2 - ) + invalid!(Some(*span), "should be a string"); + // Reconstruct + vals[index] = Value::string( + config.completion_algorithm.clone(), + *span, + ); } } "case_sensitive" => { - if let Ok(b) = value.as_bool() { - config.case_sensitive_completions = b; - } else { - eprintln!("$env.config.{}.{} is not a bool", key, key2) - } + try_bool!( + cols, + vals, + index, + span, + case_sensitive_completions + ) } "external" => { - if let Ok((cols, inner_vals)) = value.as_record() { - for (key3, value) in cols.iter().zip(inner_vals) { - let key3 = key3.as_str(); + if let Value::Record { cols, vals, span } = &mut vals[index] + { + for index in (0..cols.len()).rev() { + let value = &vals[index]; + let key3 = cols[index].as_str(); match key3 { "max_results" => { - if let Ok(i) = value.as_integer() { - config - .max_external_completion_results = - i; - } else { - eprintln!("$env.config.{}.{}.{} is not an integer", key, key2, key3) - } + try_int!( + cols, + vals, + index, + span, + max_external_completion_results + ) } "completer" => { if let Ok(v) = value.as_block() { @@ -406,131 +550,247 @@ impl Value { match value { Value::Nothing { .. } => {} _ => { - eprintln!("$env.config.{}.{}.{} is not a block or null", key, key2, key3) + invalid!( + Some(*span), + "should be a block or null" + ); + // Reconstruct + vals[index] = reconstruct_external_completer!( + span + ); } } } } "enable" => { - if let Ok(b) = value.as_bool() { - config.enable_external_completion = b; - } else { - eprintln!("$env.config.{}.{}.{} is not a bool", key, key2, key3) - } + try_bool!( + cols, + vals, + index, + span, + enable_external_completion + ) } x => { - eprintln!("$env.config.{}.{}.{} is an unknown config setting", key, key2, x) + invalid_key!( + cols, + vals, + index, + value.span().ok(), + "$env.config.{key}.{key2}.{x} is an unknown config setting" + ); } } } } else { - eprintln!( - "$env.config.{}.{} is not a record", - key, key2 - ); + invalid!(Some(*span), "should be a record"); + // Reconstruct + vals[index] = reconstruct_external!(span); } } x => { - eprintln!( - "$env.config.{}.{} is an unknown config setting", - key, x - ) + invalid_key!( + cols, + vals, + index, + value.span().ok(), + "$env.config.{key}.{x} is an unknown config setting" + ); } } } } else { - eprintln!("$env.config.{} is not a record", key) + invalid!(vals[index].span().ok(), "should be a record"); + // Reconstruct record + vals[index] = Value::record( + vec![ + "quick".into(), + "partial".into(), + "algorithm".into(), + "case_sensitive".into(), + "external".into(), + ], + vec![ + Value::boolean(config.quick_completions, *span), + Value::boolean(config.partial_completions, *span), + Value::string(config.completion_algorithm.clone(), *span), + Value::boolean(config.case_sensitive_completions, *span), + reconstruct_external!(span), + ], + *span, + ); } } "table" => { - if let Ok((cols, inner_vals)) = value.as_record() { - for (key2, value) in cols.iter().zip(inner_vals) { - let key2 = key2.as_str(); + macro_rules! reconstruct_index_mode { + ($span:expr) => { + Value::string( + match config.table_index_mode { + TableIndexMode::Always => "always", + TableIndexMode::Never => "never", + TableIndexMode::Auto => "auto", + }, + *$span, + ) + }; + } + macro_rules! reconstruct_trim_strategy { + ($span:expr) => { + match &config.trim_strategy { + TrimStrategy::Wrap { try_to_keep_words } => Value::record( + vec![ + "methodology".into(), + "wrapping_try_keep_words".into(), + ], + vec![ + Value::string("wrapping", *$span), + Value::boolean(*try_to_keep_words, *$span), + ], + *$span, + ), + TrimStrategy::Truncate { suffix } => Value::record( + vec!["methodology".into(), "truncating_suffix".into()], + match suffix { + Some(s) => vec![ + Value::string("truncating", *$span), + Value::string(s.clone(), *$span), + ], + None => vec![ + Value::string("truncating", *$span), + Value::Nothing { span: *span }, + ], + }, + *$span, + ), + } + }; + } + if let Value::Record { cols, vals, span } = &mut vals[index] { + for index in (0..cols.len()).rev() { + let value = &vals[index]; + let key2 = cols[index].as_str(); match key2 { "mode" => { if let Ok(v) = value.as_string() { config.table_mode = v; } else { - eprintln!( - "$env.config.{}.{} is not a string", - key, key2 - ) + invalid!(Some(*span), "should be a string"); + vals[index] = + Value::string(config.table_mode.clone(), *span); } } "index_mode" => { if let Ok(b) = value.as_string() { let val_str = b.to_lowercase(); match val_str.as_ref() { - "always" => config.table_index_mode = TableIndexMode::Always, - "never" => config.table_index_mode = TableIndexMode::Never, - "auto" => config.table_index_mode = TableIndexMode::Auto, - _ => eprintln!( - "unrecognized $env.config.{}.{} '{val_str}'; expected either 'never', 'always' or 'auto'", - key, key2 - ), + "always" => { + config.table_index_mode = TableIndexMode::Always + } + "never" => { + config.table_index_mode = TableIndexMode::Never + } + "auto" => { + config.table_index_mode = TableIndexMode::Auto + } + _ => { + invalid!( Some(*span), + "unrecognized $env.config.{key}.{key2} '{val_str}'; expected either 'never', 'always' or 'auto'" + ); + vals[index] = reconstruct_index_mode!(span); + } } } else { - eprintln!( - "$env.config.{}.{} is not a string", - key, key2 - ) + invalid!(Some(*span), "should be a string"); + vals[index] = reconstruct_index_mode!(span); } } "trim" => { - config.trim_strategy = - try_parse_trim_strategy(value, &config)? + match try_parse_trim_strategy(value, &config, &mut errors) { + Ok(v) => config.trim_strategy = v, + Err(e) => { + // try_parse_trim_strategy() already adds its own errors + errors.push(e); + vals[index] = reconstruct_trim_strategy!(span); + } + } } x => { - eprintln!( - "$env.config.{}.{} is an unknown config setting", - key, x - ) + invalid_key!( + cols, + vals, + index, + value.span().ok(), + "$env.config.{key}.{x} is an unknown config setting" + ); } } } } else { - eprintln!("$env.config.{} is not a record", key) + invalid!(vals[index].span().ok(), "should be a record"); + // Reconstruct + vals[index] = Value::record( + vec!["mode".into(), "index_mode".into(), "trim".into()], + vec![ + Value::string(config.table_mode.clone(), *span), + reconstruct_index_mode!(span), + reconstruct_trim_strategy!(span), + ], + *span, + ) } } "filesize" => { - if let Ok((cols, inner_vals)) = value.as_record() { - for (key2, value) in cols.iter().zip(inner_vals) { - let key2 = key2.as_str(); + if let Value::Record { cols, vals, span } = &mut vals[index] { + for index in (0..cols.len()).rev() { + let value = &vals[index]; + let key2 = cols[index].as_str(); match key2 { "metric" => { - if let Ok(b) = value.as_bool() { - config.filesize_metric = b; - } else { - eprintln!("$env.config.{}.{} is not a bool", key, key2) - } + try_bool!(cols, vals, index, span, filesize_metric) } "format" => { if let Ok(v) = value.as_string() { config.filesize_format = v.to_lowercase(); } else { - eprintln!( - "$env.config.{}.{} is not a string", - key, key2 - ) + invalid!(Some(*span), "should be a string"); + // Reconstruct + vals[index] = Value::string( + config.filesize_format.clone(), + *span, + ); } } x => { - eprintln!( - "$env.config.{}.{} is an unknown config setting", - key, x - ) + invalid_key!( + cols, + vals, + index, + value.span().ok(), + "$env.config.{key}.{x} is an unknown config setting" + ); } } } } else { - eprintln!("$env.config.{} is not a record", key) + invalid!(vals[index].span().ok(), "should be a record"); + // Reconstruct + vals[index] = Value::record( + vec!["metric".into(), "format".into()], + vec![ + Value::boolean(config.filesize_metric, *span), + Value::string(config.filesize_format.clone(), *span), + ], + *span, + ); } } "explore" => { if let Ok(map) = create_map(value, &config) { config.explore = map; } else { - eprintln!("$env.config.{} is not a record", key) + invalid!(vals[index].span().ok(), "should be a record"); + // Reconstruct + vals[index] = Value::record_from_hashmap(&config.explore, *span); } } // Misc. options @@ -538,15 +798,13 @@ impl Value { if let Ok(map) = create_map(value, &config) { config.color_config = map; } else { - eprintln!("$env.config.color_config is not a record") + invalid!(vals[index].span().ok(), "should be a record"); + // Reconstruct + vals[index] = Value::record_from_hashmap(&config.color_config, *span); } } "use_grid_icons" => { - if let Ok(b) = value.as_bool() { - config.use_grid_icons = b; - } else { - eprintln!("$env.config.{} is not a bool", key) - } + try_bool!(cols, vals, index, span, use_grid_icons); } "footer_mode" => { if let Ok(b) = value.as_string() { @@ -561,102 +819,195 @@ impl Value { }, }; } else { - eprintln!("$env.config.{} is not a string", key) + invalid!(Some(*span), "should be a string"); + // Reconstruct + vals[index] = Value::String { + val: match config.footer_mode { + FooterMode::Auto => "auto".into(), + FooterMode::Never => "never".into(), + FooterMode::Always => "always".into(), + FooterMode::RowCount(number) => number.to_string(), + }, + span: *span, + }; } } "float_precision" => { - if let Ok(i) = value.as_integer() { - config.float_precision = i; - } else { - eprintln!("$env.config.{} is not an integer", key) - } + try_int!(cols, vals, index, span, float_precision); } "use_ansi_coloring" => { - if let Ok(b) = value.as_bool() { - config.use_ansi_coloring = b; - } else { - eprintln!("$env.config.{} is not a bool", key) - } + try_bool!(cols, vals, index, span, use_ansi_coloring); } "edit_mode" => { if let Ok(v) = value.as_string() { config.edit_mode = v.to_lowercase(); } else { - eprintln!("$env.config.{} is not a string", key) + invalid!(Some(*span), "should be a string"); + // Reconstruct + vals[index] = Value::string(config.edit_mode.clone(), *span); } } "log_level" => { if let Ok(v) = value.as_string() { config.log_level = v.to_lowercase(); } else { - eprintln!("$env.config.{} is not a string", key) + invalid!(Some(*span), "should be a string"); + // Reconstruct + vals[index] = Value::string(config.log_level.clone(), *span); } } "menus" => match create_menus(value) { Ok(map) => config.menus = map, Err(e) => { - eprintln!("$env.config.{} is not a valid list of menus", key); - eprintln!("{:?}", e); + invalid!(Some(*span), "should be a valid list of menus"); + errors.push(e); + // Reconstruct + vals[index] = Value::List { + vals: config + .menus + .iter() + .map( + |ParsedMenu { + name, + only_buffer_difference, + marker, + style, + menu_type, // WARNING: this is not the same name as what is used in Config.nu! ("type") + source, + }| { + Value::Record { + cols: vec![ + "name".into(), + "only_buffer_difference".into(), + "marker".into(), + "style".into(), + "type".into(), + "source".into(), + ], + vals: vec![ + name.clone(), + only_buffer_difference.clone(), + marker.clone(), + style.clone(), + menu_type.clone(), + source.clone(), + ], + span: *span, + } + }, + ) + .collect(), + span: *span, + } } }, "keybindings" => match create_keybindings(value) { Ok(keybindings) => config.keybindings = keybindings, Err(e) => { - eprintln!("$env.config.{} is not a valid keybindings list", key); - eprintln!("{:?}", e); + invalid!(Some(*span), "should be a valid keybindings list"); + errors.push(e); + // Reconstruct + vals[index] = Value::List { + vals: config + .keybindings + .iter() + .map( + |ParsedKeybinding { + modifier, + keycode, + mode, + event, + }| { + Value::Record { + cols: vec![ + "modifier".into(), + "keycode".into(), + "mode".into(), + "event".into(), + ], + vals: vec![ + modifier.clone(), + keycode.clone(), + mode.clone(), + event.clone(), + ], + span: *span, + } + }, + ) + .collect(), + span: *span, + } } }, "hooks" => match create_hooks(value) { Ok(hooks) => config.hooks = hooks, Err(e) => { - eprintln!("$env.config.{} is not a valid hooks list", key); - eprintln!("{:?}", e); + invalid!(Some(*span), "should be a valid hooks list"); + errors.push(e); + // Reconstruct + let mut hook_cols = vec![]; + let mut hook_vals = vec![]; + match &config.hooks.pre_prompt { + Some(v) => { + hook_cols.push("pre_prompt".into()); + hook_vals.push(v.clone()); + } + None => (), + }; + match &config.hooks.pre_execution { + Some(v) => { + hook_cols.push("pre_execution".into()); + hook_vals.push(v.clone()); + } + None => (), + }; + match &config.hooks.env_change { + Some(v) => { + hook_cols.push("env_change".into()); + hook_vals.push(v.clone()); + } + None => (), + }; + match &config.hooks.display_output { + Some(v) => { + hook_cols.push("display_output".into()); + hook_vals.push(v.clone()); + } + None => (), + }; + vals.push(Value::Record { + cols: hook_cols, + vals: hook_vals, + span: *span, + }); } }, "shell_integration" => { - if let Ok(b) = value.as_bool() { - config.shell_integration = b; - } else { - eprintln!("$env.config.{} is not a bool", key); - } + try_bool!(cols, vals, index, span, shell_integration); } "buffer_editor" => { if let Ok(v) = value.as_string() { config.buffer_editor = v.to_lowercase(); } else { - eprintln!("$env.config.{} is not a string", key); + invalid!(Some(*span), "should be a string"); } } "show_banner" => { - if let Ok(b) = value.as_bool() { - config.show_banner = b; - } else { - eprintln!("$env.config.{} is not a bool", key); - } + try_bool!(cols, vals, index, span, show_banner); } "render_right_prompt_on_last_line" => { - if let Ok(b) = value.as_bool() { - config.render_right_prompt_on_last_line = b; - } else { - eprintln!("$env.config.{} is not a bool", key); - } + try_bool!(cols, vals, index, span, render_right_prompt_on_last_line); } // Legacy config options (deprecated as of 2022-11-02) + // Legacy options do NOT reconstruct their values on error "use_ls_colors" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.use_ls_colors = b; - } else { - eprintln!("$env.config.use_ls_colors is not a bool") - } + try_bool!(cols, vals, index, span, use_ls_colors); } "rm_always_trash" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.rm_always_trash = b; - } else { - eprintln!("$env.config.rm_always_trash is not a bool") - } + try_bool!(cols, vals, index, span, rm_always_trash); } "history_file_format" => { legacy_options_used = true; @@ -666,57 +1017,36 @@ impl Value { "sqlite" => HistoryFileFormat::Sqlite, "plaintext" => HistoryFileFormat::PlainText, _ => { - eprintln!( - "unrecognized $env.config.history_file_format '{val_str}'" + invalid!( + Some(*span), + "unrecognized $env.config.{key} '{val_str}'" ); HistoryFileFormat::PlainText } }; } else { - eprintln!("$env.config.history_file_format is not a string") + invalid!(Some(*span), "should be a string"); } } "sync_history_on_enter" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.sync_history_on_enter = b; - } else { - eprintln!("$env.config.sync_history_on_enter is not a bool") - } + try_bool!(cols, vals, index, span, sync_history_on_enter); } "max_history_size" => { legacy_options_used = true; - if let Ok(i) = value.as_i64() { - config.max_history_size = i; - } else { - eprintln!("$env.config.max_history_size is not an integer") - } + try_int!(cols, vals, index, span, max_history_size); } "quick_completions" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.quick_completions = b; - } else { - eprintln!("$env.config.quick_completions is not a bool") - } + try_bool!(cols, vals, index, span, quick_completions); } "partial_completions" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.partial_completions = b; - } else { - eprintln!("$env.config.partial_completions is not a bool") - } + try_bool!(cols, vals, index, span, partial_completions); } "max_external_completion_results" => { legacy_options_used = true; - if let Ok(i) = value.as_integer() { - config.max_external_completion_results = i; - } else { - eprintln!( - "$env.config.max_external_completion_results is not an integer" - ) - } + try_int!(cols, vals, index, span, max_external_completion_results); } "completion_algorithm" => { legacy_options_used = true; @@ -727,45 +1057,38 @@ impl Value { "prefix" => val_str, "fuzzy" => val_str, _ => { - eprintln!( - "unrecognized $env.config.completions.algorithm '{val_str}'; expected either 'prefix' or 'fuzzy'" + invalid!( Some(*span), + "unrecognized $env.config.{key} '{val_str}'; expected either 'prefix' or 'fuzzy'" ); val_str } }; } else { - eprintln!("$env.config.completion_algorithm is not a string") + invalid!(Some(*span), "should be a string"); } } "case_sensitive_completions" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.case_sensitive_completions = b; - } else { - eprintln!("$env.config.case_sensitive_completions is not a bool") - } + try_bool!(cols, vals, index, span, case_sensitive_completions); } "enable_external_completion" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.enable_external_completion = b; - } else { - eprintln!("$env.config.enable_external_completion is not a bool") - } + try_bool!(cols, vals, index, span, enable_external_completion); } - "external_completer" => { legacy_options_used = true; if let Ok(v) = value.as_block() { config.external_completer = Some(v) } + // No error here because external completers are optional. + // Idea: maybe error if this is a non-block, non-null? } "table_mode" => { legacy_options_used = true; if let Ok(v) = value.as_string() { config.table_mode = v; } else { - eprintln!("$env.config.table_mode is not a string") + invalid!(Some(*span), "should be a string"); } } "table_index_mode" => { @@ -776,75 +1099,116 @@ impl Value { "always" => config.table_index_mode = TableIndexMode::Always, "never" => config.table_index_mode = TableIndexMode::Never, "auto" => config.table_index_mode = TableIndexMode::Auto, - _ => eprintln!( - "unrecognized $env.config.table_index_mode '{val_str}'; expected either 'never', 'always' or 'auto'" - ), + _ => { + invalid!( Some(*span), + "unrecognized $env.config.table_index_mode '{val_str}'; expected either 'never', 'always' or 'auto'" + ); + } } } else { - eprintln!("$env.config.table_index_mode is not a string") + invalid!(Some(*span), "should be a string"); } } "table_trim" => { legacy_options_used = true; - config.trim_strategy = try_parse_trim_strategy(value, &config)? + match try_parse_trim_strategy(value, &config, &mut errors) { + Ok(v) => config.trim_strategy = v, + Err(e) => { + // try_parse_trim_strategy() already calls eprintln!() on error + cols.remove(index); + vals.remove(index); + errors.push(e); + } + } } "show_clickable_links_in_ls" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.show_clickable_links_in_ls = b; - } else { - eprintln!("$env.config.show_clickable_links_in_ls is not a bool") - } + try_bool!(cols, vals, index, span, show_clickable_links_in_ls); } "cd_with_abbreviations" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.cd_with_abbreviations = b; - } else { - eprintln!("$env.config.cd_with_abbreviations is not a bool") - } + try_bool!(cols, vals, index, span, cd_with_abbreviations); } "filesize_metric" => { legacy_options_used = true; - if let Ok(b) = value.as_bool() { - config.filesize_metric = b; - } else { - eprintln!("$env.config.filesize_metric is not a bool") - } + try_bool!(cols, vals, index, span, filesize_metric); } "filesize_format" => { legacy_options_used = true; if let Ok(v) = value.as_string() { config.filesize_format = v.to_lowercase(); } else { - eprintln!("$env.config.filesize_format is not a string") + invalid!(Some(*span), "should be a string"); } } // End legacy options x => { - eprintln!("$env.config.{} is an unknown config setting", x) + invalid_key!( + cols, + vals, + index, + value.span().ok(), + "$env.config.{x} is an unknown config setting" + ); } } } } else { - eprintln!("$env.config is not a record"); + return ( + config, + Some(ShellError::GenericError( + "Error while applying config changes".into(), + "$env.config is not a record".into(), + self.span().ok(), + None, + vec![], + )), + ); } if legacy_options_used { + // This is a notification message, not an error. eprintln!( r#"The format of $env.config has recently changed, and several options have been grouped into sub-records. You may need to update your config.nu file. Please consult https://www.nushell.sh/blog/2022-11-29-nushell-0.72.html for details. Support for the old format will be removed in an upcoming Nu release."# ); } - Ok(config) + // Return the config and the vec of errors. + ( + config, + if !errors.is_empty() { + // Because the config was iterated in reverse, these errors + // need to be reversed, too. + errors.reverse(); + Some(ShellError::GenericError( + "Config record contains invalid values or unknown settings".into(), + // Without a span, this second string is ignored. + "".into(), + None, + None, + errors, + )) + } else { + None + }, + ) } } -fn try_parse_trim_strategy(value: &Value, config: &Config) -> Result { +fn try_parse_trim_strategy( + value: &Value, + config: &Config, + errors: &mut Vec, +) -> Result { let map = create_map(value, config).map_err(|e| { - eprintln!("$env.config.table.trim is not a record"); - e + ShellError::GenericError( + "Error while applying config changes".into(), + "$env.config.table.trim is not a record".into(), + value.span().ok(), + Some("Please consult the documentation for configuring Nushell.".into()), + vec![e], + ) })?; let mut methodology = match map.get("methodology") { @@ -853,7 +1217,13 @@ fn try_parse_trim_strategy(value: &Value, config: &Config) -> Result return Ok(TRIM_STRATEGY_DEFAULT), }, None => { - eprintln!("$env.config.table.trim.methodology was not provided"); + errors.push(ShellError::GenericError( + "Error while applying config changes".into(), + "$env.config.table.trim.methodology was not provided".into(), + value.span().ok(), + Some("Please consult the documentation for configuring Nushell.".into()), + vec![], + )); return Ok(TRIM_STRATEGY_DEFAULT); } }; @@ -864,7 +1234,13 @@ fn try_parse_trim_strategy(value: &Value, config: &Config) -> Result Result Result { "display_output" => hooks.display_output = Some(vals[idx].clone()), x => { return Err(ShellError::UnsupportedConfigValue( - "'pre_prompt', 'pre_execution', 'env_change'".to_string(), + "'pre_prompt', 'pre_execution', 'env_change', 'display_output'" + .to_string(), x.to_string(), *span, )); diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index b2fe080efa..abb6ea9014 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -236,10 +236,18 @@ impl EngineState { // Updating existing overlay for (k, v) in env.drain() { if k == "config" { - self.config = v.clone().into_config().unwrap_or_default(); + // Don't insert the record as the "config" env var as-is. + // Instead, mutate a clone of it with into_config(), and put THAT in env_vars. + let mut new_record = v.clone(); + let (config, error) = new_record.into_config(&self.config); + self.config = config; + env_vars.insert(k, new_record); + if let Some(e) = error { + return Err(e); + } + } else { + env_vars.insert(k, v); } - - env_vars.insert(k, v); } } else { // Pushing a new overlay diff --git a/crates/nu-protocol/src/value/mod.rs b/crates/nu-protocol/src/value/mod.rs index 7d071948f6..d1070cbd50 100644 --- a/crates/nu-protocol/src/value/mod.rs +++ b/crates/nu-protocol/src/value/mod.rs @@ -1315,6 +1315,24 @@ impl Value { Value::Bool { val, span } } + pub fn record(cols: Vec, vals: Vec, span: Span) -> Value { + Value::Record { cols, vals, span } + } + + pub fn record_from_hashmap(map: &HashMap, span: Span) -> Value { + let mut cols = vec![]; + let mut vals = vec![]; + for (key, val) in map.iter() { + cols.push(key.clone()); + vals.push(val.clone()); + } + Value::record(cols, vals, span) + } + + pub fn list(vals: Vec, span: Span) -> Value { + Value::List { vals, span } + } + /// Note: Only use this for test data, *not* live data, as it will point into unknown source /// when used in errors. pub fn test_string(s: impl Into) -> Value { diff --git a/crates/nu-protocol/tests/into_config.rs b/crates/nu-protocol/tests/into_config.rs new file mode 100644 index 0000000000..d06ae3e838 --- /dev/null +++ b/crates/nu-protocol/tests/into_config.rs @@ -0,0 +1,101 @@ +use nu_test_support::{nu, nu_repl_code}; + +#[test] +fn config_is_mutable() { + let actual = nu!(cwd: ".", nu_repl_code(&[r"let-env config = { ls: { clickable_links: true } }", + "$env.config.ls.clickable_links = false;", + "$env.config.ls.clickable_links"])); + + assert_eq!(actual.out, "false"); +} + +#[test] +fn config_preserved_after_do() { + let actual = nu!(cwd: ".", nu_repl_code(&[r"let-env config = { ls: { clickable_links: true } }", + "do -i { $env.config.ls.clickable_links = false }", + "$env.config.ls.clickable_links"])); + + assert_eq!(actual.out, "true"); +} + +#[test] +fn config_affected_when_mutated() { + let actual = nu!(cwd: ".", nu_repl_code(&[r#"let-env config = { filesize: { metric: false, format:"auto" } }"#, + r#"$env.config = { filesize: { metric: true, format:"auto" } }"#, + "20mib | into string"])); + + assert_eq!(actual.out, "21.0 MB"); +} + +#[test] +fn config_affected_when_deep_mutated() { + let actual = nu!(cwd: "crates/nu-utils/src/sample_config", nu_repl_code(&[ + r#"source default_config.nu"#, + r#"$env.config.filesize.metric = true"#, + r#"20mib | into string"#])); + + assert_eq!(actual.out, "21.0 MB"); +} + +#[test] +fn config_add_unsupported_key() { + let actual = nu!(cwd: "crates/nu-utils/src/sample_config", nu_repl_code(&[ + r#"source default_config.nu"#, + r#"$env.config.foo = 2"#, + r#";"#])); + + assert!(actual + .err + .contains("$env.config.foo is an unknown config setting")); +} + +#[test] +fn config_add_unsupported_type() { + let actual = nu!(cwd: "crates/nu-utils/src/sample_config", nu_repl_code(&[r#"source default_config.nu"#, + r#"$env.config.ls = '' "#, + r#";"#])); + + assert!(actual.err.contains("should be a record")); +} + +#[test] +fn config_add_unsupported_value() { + let actual = nu!(cwd: "crates/nu-utils/src/sample_config", nu_repl_code(&[r#"source default_config.nu"#, + r#"$env.config.history.file_format = ''"#, + r#";"#])); + + assert!(actual.err.contains( + "unrecognized $env.config.history.file_format ''; expected either 'sqlite' or 'plaintext'" + )); +} + +#[test] +#[ignore = "Figure out how to make test_bins::nu_repl() continue execution after shell errors"] +fn config_unsupported_key_reverted() { + let actual = nu!(cwd: "crates/nu-utils/src/sample_config", nu_repl_code(&[r#"source default_config.nu"#, + r#"$env.config.foo = 1"#, + r#"'foo' in $env.config"#])); + + assert_eq!(actual.out, "false"); +} + +#[test] +#[ignore = "Figure out how to make test_bins::nu_repl() continue execution after shell errors"] +fn config_unsupported_type_reverted() { + let actual = nu!(cwd: "crates/nu-utils/src/sample_config", nu_repl_code(&[r#" source default_config.nu"#, + r#"$env.config.ls = ''"#, + r#"$env.config.ls | describe"#])); + + assert_eq!(actual.out, "record"); +} + +#[test] +#[ignore = "Figure out how to make test_bins::nu_repl() continue execution after errors"] +fn config_unsupported_value_reverted() { + let actual = nu!(cwd: "crates/nu-utils/src/sample_config", nu_repl_code(&[r#" source default_config.nu"#, + r#"$env.config.history.file_format = 'plaintext'"#, + r#"$env.config.history.file_format = ''"#, + r#"$env.config.history.file_format | to json"#])); + + assert_eq!(actual.out, "\"plaintext\""); +}