From 24d2c8dd8e169c8bf0c01dfca3974c3149a19f23 Mon Sep 17 00:00:00 2001 From: Stefan Holderbach Date: Tue, 26 Mar 2024 12:12:25 +0100 Subject: [PATCH] Follow API guidelines for public types (#12283) # Description Follow the [API guideline naming conventions](https://rust-lang.github.io/api-guidelines/naming.html) also for our externally exposed types (See [`clippy::wrong_self_convention`](https://rust-lang.github.io/rust-clippy/master/index.html#/wrong_self_convention) with [`avoid-breaking-exported-api = false`](https://doc.rust-lang.org/clippy/lint_configuration.html#avoid-breaking-exported-api) ) Also be a good citizen around doccomments - **Fix `Unit::to_value` to `Unit::build_value`** - **Fix `PipelineData::is_external_failed` to `check_external_failed`** - **Fix doccomment on `check_external_failed`** - **Fix `Value::into_config` naming to `parse_as_config`** - **Document `Value::parse_as_config`** # Plugin-Author-Facing Changes See renames above --- crates/nu-cmd-lang/src/core_commands/try_.rs | 2 +- crates/nu-engine/src/env.rs | 2 +- crates/nu-engine/src/eval.rs | 2 +- crates/nu-protocol/src/ast/unit.rs | 2 +- crates/nu-protocol/src/config/mod.rs | 11 +++++++++-- crates/nu-protocol/src/engine/engine_state.rs | 2 +- crates/nu-protocol/src/engine/pattern_match.rs | 2 +- crates/nu-protocol/src/eval_base.rs | 2 +- crates/nu-protocol/src/pipeline_data/mod.rs | 14 ++++++++------ 9 files changed, 24 insertions(+), 15 deletions(-) diff --git a/crates/nu-cmd-lang/src/core_commands/try_.rs b/crates/nu-cmd-lang/src/core_commands/try_.rs index fec1ec46d8..449ab6f72d 100644 --- a/crates/nu-cmd-lang/src/core_commands/try_.rs +++ b/crates/nu-cmd-lang/src/core_commands/try_.rs @@ -65,7 +65,7 @@ impl Command for Try { } // external command may fail to run Ok(pipeline) => { - let (pipeline, external_failed) = pipeline.is_external_failed(); + let (pipeline, external_failed) = pipeline.check_external_failed(); if external_failed { // Because external command errors aren't "real" errors, // (unless do -c is in effect) diff --git a/crates/nu-engine/src/env.rs b/crates/nu-engine/src/env.rs index e927a86023..8a83b6e4fe 100644 --- a/crates/nu-engine/src/env.rs +++ b/crates/nu-engine/src/env.rs @@ -353,7 +353,7 @@ pub fn find_in_dirs_env( /// is the canonical way to fetch config at runtime when you have Stack available. pub fn get_config(engine_state: &EngineState, stack: &Stack) -> Config { if let Some(mut config_record) = stack.get_env_var(engine_state, "config") { - config_record.into_config(engine_state.get_config()).0 + config_record.parse_as_config(engine_state.get_config()).0 } else { engine_state.get_config().clone() } diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index fb17815466..bd9daa6976 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -314,7 +314,7 @@ pub fn eval_expression_with_input( // Try to catch and detect if external command runs to failed. fn might_consume_external_result(input: PipelineData) -> (PipelineData, bool) { - input.is_external_failed() + input.check_external_failed() } fn eval_redirection( diff --git a/crates/nu-protocol/src/ast/unit.rs b/crates/nu-protocol/src/ast/unit.rs index 3db631e9b1..0e51d8978f 100644 --- a/crates/nu-protocol/src/ast/unit.rs +++ b/crates/nu-protocol/src/ast/unit.rs @@ -32,7 +32,7 @@ pub enum Unit { } impl Unit { - pub fn to_value(&self, size: i64, span: Span) -> Result { + pub fn build_value(self, size: i64, span: Span) -> Result { match self { Unit::Byte => Ok(Value::filesize(size, span)), Unit::Kilobyte => Ok(Value::filesize(size * 1000, span)), diff --git a/crates/nu-protocol/src/config/mod.rs b/crates/nu-protocol/src/config/mod.rs index dbdba95a6c..72c1c9a7c8 100644 --- a/crates/nu-protocol/src/config/mod.rs +++ b/crates/nu-protocol/src/config/mod.rs @@ -172,9 +172,16 @@ impl Default for Config { } impl Value { - pub fn into_config(&mut self, config: &Config) -> (Config, Option) { + /// Parse the given [`Value`] as a configuration record, and recover encountered mistakes + /// + /// If any given (sub)value is detected as impossible, this value will be restored to the value + /// in `existing_config`, thus mutates `self`. + /// + /// Returns a new [`Config`] (that is in a valid state) and if encountered the [`ShellError`] + /// containing all observed inner errors. + pub fn parse_as_config(&mut self, existing_config: &Config) -> (Config, Option) { // Clone the passed-in config rather than mutating it. - let mut config = config.clone(); + let mut config = existing_config.clone(); // Vec for storing errors. Current Nushell behaviour (Dec 2022) is that having some typo // like `"always_trash": tru` in your config.nu's `$env.config` record shouldn't abort all diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index a53e4c0db9..a2271e900a 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -295,7 +295,7 @@ impl EngineState { // 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); + let (config, error) = new_record.parse_as_config(&self.config); self.config = Arc::new(config); config_updated = true; env_vars.insert(k, new_record); diff --git a/crates/nu-protocol/src/engine/pattern_match.rs b/crates/nu-protocol/src/engine/pattern_match.rs index 8626a89943..8c1266c1b5 100644 --- a/crates/nu-protocol/src/engine/pattern_match.rs +++ b/crates/nu-protocol/src/engine/pattern_match.rs @@ -145,7 +145,7 @@ impl Matcher for Pattern { let span = unit.span; if let Expr::Int(size) = amount.expr { - match &unit.item.to_value(size, span) { + match &unit.item.build_value(size, span) { Ok(v) => v == value, _ => false, } diff --git a/crates/nu-protocol/src/eval_base.rs b/crates/nu-protocol/src/eval_base.rs index f24f860482..84ca009673 100644 --- a/crates/nu-protocol/src/eval_base.rs +++ b/crates/nu-protocol/src/eval_base.rs @@ -136,7 +136,7 @@ pub trait Eval { Expr::String(s) => Ok(Value::string(s.clone(), expr.span)), Expr::Nothing => Ok(Value::nothing(expr.span)), Expr::ValueWithUnit(e, unit) => match Self::eval::(state, mut_state, e)? { - Value::Int { val, .. } => unit.item.to_value(val, unit.span), + Value::Int { val, .. } => unit.item.build_value(val, unit.span), x => Err(ShellError::CantConvert { to_type: "unit value".into(), from_type: x.get_type().to_string(), diff --git a/crates/nu-protocol/src/pipeline_data/mod.rs b/crates/nu-protocol/src/pipeline_data/mod.rs index 49ea720713..857cab36ed 100644 --- a/crates/nu-protocol/src/pipeline_data/mod.rs +++ b/crates/nu-protocol/src/pipeline_data/mod.rs @@ -699,14 +699,16 @@ impl PipelineData { } } - /// Try to catch external stream exit status and detect if it runs to failed. + /// Try to catch the external stream exit status and detect if it failed. /// - /// This is useful to commands with semicolon, we can detect errors early to avoid - /// commands after semicolon running. + /// This is useful for external commands with semicolon, we can detect errors early to avoid + /// commands after the semicolon running. /// - /// Returns self and a flag indicates if the external stream runs to failed. - /// If `self` is not Pipeline::ExternalStream, the flag will be false. - pub fn is_external_failed(self) -> (Self, bool) { + /// Returns `self` and a flag that indicates if the external stream run failed. If `self` is + /// not [`PipelineData::ExternalStream`], the flag will be `false`. + /// + /// Currently this will consume an external stream to completion. + pub fn check_external_failed(self) -> (Self, bool) { let mut failed_to_run = false; // Only need ExternalStream without redirecting output. // It indicates we have no more commands to execute currently.