From 3e3cb15f3d404cfb8a5e30d62e1dafd8ddf9f075 Mon Sep 17 00:00:00 2001 From: Jonathan Turner Date: Thu, 2 Jan 2020 20:07:17 +1300 Subject: [PATCH] Yet more ununwraps (#1150) --- .../nu-parser/src/hir/baseline_parse/tests.rs | 8 +- crates/nu-parser/src/hir/syntax_shape.rs | 42 ++++++--- src/commands/classified/internal.rs | 2 +- src/commands/split_by.rs | 11 ++- src/commands/to_json.rs | 28 ++++-- src/context.rs | 32 +++++-- src/data/base.rs | 88 +++++++++++-------- 7 files changed, 135 insertions(+), 76 deletions(-) diff --git a/crates/nu-parser/src/hir/baseline_parse/tests.rs b/crates/nu-parser/src/hir/baseline_parse/tests.rs index 82fa9627a1..e73f1b8e67 100644 --- a/crates/nu-parser/src/hir/baseline_parse/tests.rs +++ b/crates/nu-parser/src/hir/baseline_parse/tests.rs @@ -103,11 +103,11 @@ impl TestRegistry { } impl SignatureRegistry for TestRegistry { - fn has(&self, name: &str) -> bool { - self.signatures.contains_key(name) + fn has(&self, name: &str) -> Result { + Ok(self.signatures.contains_key(name)) } - fn get(&self, name: &str) -> Option { - self.signatures.get(name).cloned() + fn get(&self, name: &str) -> Result, ShellError> { + Ok(self.signatures.get(name).cloned()) } } diff --git a/crates/nu-parser/src/hir/syntax_shape.rs b/crates/nu-parser/src/hir/syntax_shape.rs index 4d900ec4d9..839efd2a3c 100644 --- a/crates/nu-parser/src/hir/syntax_shape.rs +++ b/crates/nu-parser/src/hir/syntax_shape.rs @@ -127,8 +127,8 @@ impl ExpandExpression for SyntaxShape { } pub trait SignatureRegistry { - fn has(&self, name: &str) -> bool; - fn get(&self, name: &str) -> Option; + fn has(&self, name: &str) -> Result; + fn get(&self, name: &str) -> Result, ShellError>; } #[derive(Getters, new)] @@ -673,16 +673,26 @@ impl FallibleColorSyntax for CommandHeadShape { UnspannedAtomicToken::Word { text } => { let name = text.slice(context.source); - if context.registry.has(name) { + if context.registry.has(name)? { // If the registry has the command, color it as an internal command token_nodes.color_shape(FlatShape::InternalCommand.spanned(text)); - let signature = context.registry.get(name).ok_or_else(|| { - ShellError::labeled_error( - "Internal error: could not load signature from registry", - "could not load from registry", - text, - ) - })?; + let signature = context + .registry + .get(name) + .map_err(|_| { + ShellError::labeled_error( + "Internal error: could not load signature from registry", + "could not load from registry", + text, + ) + })? + .ok_or_else(|| { + ShellError::labeled_error( + "Internal error: could not load signature from registry", + "could not load from registry", + text, + ) + })?; Ok(CommandHeadKind::Internal(signature)) } else { // Otherwise, color it as an external command @@ -721,10 +731,14 @@ impl ExpandSyntax for CommandHeadShape { }, UnspannedToken::Bare => { let name = token_span.slice(context.source); - if context.registry.has(name) { - let signature = context.registry.get(name).ok_or_else(|| { - ParseError::internal_error(name.spanned(token_span)) - })?; + if context.registry.has(name)? { + let signature = context + .registry + .get(name) + .map_err(|_| ParseError::internal_error(name.spanned(token_span)))? + .ok_or_else(|| { + ParseError::internal_error(name.spanned(token_span)) + })?; CommandSignature::Internal(signature.spanned(token_span)) } else { CommandSignature::External(token_span) diff --git a/src/commands/classified/internal.rs b/src/commands/classified/internal.rs index b11ddb58f7..46b9da2513 100644 --- a/src/commands/classified/internal.rs +++ b/src/commands/classified/internal.rs @@ -26,7 +26,7 @@ pub(crate) async fn run_internal_command( let result = { context.run_command( - internal_command, + internal_command?, command.name_tag.clone(), command.args.clone(), &source, diff --git a/src/commands/split_by.rs b/src/commands/split_by.rs index 9af056c0ad..47f1588a48 100644 --- a/src/commands/split_by.rs +++ b/src/commands/split_by.rs @@ -156,6 +156,7 @@ mod tests { use crate::commands::group_by::group; use crate::commands::split_by::split; use indexmap::IndexMap; + use nu_errors::ShellError; use nu_protocol::{UntaggedValue, Value}; use nu_source::*; @@ -171,9 +172,9 @@ mod tests { UntaggedValue::table(list).into_untagged_value() } - fn nu_releases_grouped_by_date() -> Value { + fn nu_releases_grouped_by_date() -> Result { let key = String::from("date").tagged_unknown(); - group(&key, nu_releases_commiters(), Tag::unknown()).unwrap() + group(&key, nu_releases_commiters(), Tag::unknown()) } fn nu_releases_commiters() -> Vec { @@ -209,11 +210,11 @@ mod tests { } #[test] - fn splits_inner_tables_by_key() { + fn splits_inner_tables_by_key() -> Result<(), ShellError> { let for_key = String::from("country").tagged_unknown(); assert_eq!( - split(&for_key, &nu_releases_grouped_by_date(), Tag::unknown()).unwrap(), + split(&for_key, &nu_releases_grouped_by_date()?, Tag::unknown())?, UntaggedValue::row(indexmap! { "EC".into() => row(indexmap! { "August 23-2019".into() => table(&[ @@ -250,6 +251,8 @@ mod tests { }) }).into_untagged_value() ); + + Ok(()) } #[test] diff --git a/src/commands/to_json.rs b/src/commands/to_json.rs index 929fa8f195..e824551600 100644 --- a/src/commands/to_json.rs +++ b/src/commands/to_json.rs @@ -39,12 +39,28 @@ pub fn value_to_json_value(v: &Value) -> Result { UntaggedValue::Primitive(Primitive::Date(d)) => serde_json::Value::String(d.to_string()), UntaggedValue::Primitive(Primitive::EndOfStream) => serde_json::Value::Null, UntaggedValue::Primitive(Primitive::BeginningOfStream) => serde_json::Value::Null, - UntaggedValue::Primitive(Primitive::Decimal(f)) => serde_json::Value::Number( - serde_json::Number::from_f64( - f.to_f64().expect("TODO: What about really big decimals?"), - ) - .unwrap(), - ), + UntaggedValue::Primitive(Primitive::Decimal(f)) => { + if let Some(f) = f.to_f64() { + if let Some(num) = serde_json::Number::from_f64( + f.to_f64().expect("TODO: What about really big decimals?"), + ) { + serde_json::Value::Number(num) + } else { + return Err(ShellError::labeled_error( + "Could not convert value to decimal number", + "could not convert to decimal", + &v.tag, + )); + } + } else { + return Err(ShellError::labeled_error( + "Could not convert value to decimal number", + "could not convert to decimal", + &v.tag, + )); + } + } + UntaggedValue::Primitive(Primitive::Int(i)) => { serde_json::Value::Number(serde_json::Number::from(CoerceInto::::coerce_into( i.tagged(&v.tag), diff --git a/src/context.rs b/src/context.rs index 128ac519e8..73645a2b16 100644 --- a/src/context.rs +++ b/src/context.rs @@ -17,13 +17,25 @@ pub struct CommandRegistry { } impl SignatureRegistry for CommandRegistry { - fn has(&self, name: &str) -> bool { - let registry = self.registry.lock().unwrap(); - registry.contains_key(name) + fn has(&self, name: &str) -> Result { + if let Ok(registry) = self.registry.lock() { + Ok(registry.contains_key(name)) + } else { + Err(ShellError::untagged_runtime_error(format!( + "Could not load from registry: {}", + name + ))) + } } - fn get(&self, name: &str) -> Option { - let registry = self.registry.lock().unwrap(); - registry.get(name).map(|command| command.signature()) + fn get(&self, name: &str) -> Result, ShellError> { + if let Ok(registry) = self.registry.lock() { + Ok(registry.get(name).map(|command| command.signature())) + } else { + Err(ShellError::untagged_runtime_error(format!( + "Could not get from registry: {}", + name + ))) + } } } @@ -48,8 +60,10 @@ impl CommandRegistry { registry.get(name).cloned() } - pub(crate) fn expect_command(&self, name: &str) -> Arc { - self.get_command(name).unwrap() + pub(crate) fn expect_command(&self, name: &str) -> Result, ShellError> { + self.get_command(name).ok_or_else(|| { + ShellError::untagged_runtime_error(format!("Could not load command: {}", name)) + }) } pub(crate) fn has(&self, name: &str) -> bool { @@ -171,7 +185,7 @@ impl Context { self.registry.get_command(name) } - pub(crate) fn expect_command(&self, name: &str) -> Arc { + pub(crate) fn expect_command(&self, name: &str) -> Result, ShellError> { self.registry.expect_command(name) } diff --git a/src/data/base.rs b/src/data/base.rs index 1ecb388763..ad90a9e394 100644 --- a/src/data/base.rs +++ b/src/data/base.rs @@ -224,25 +224,28 @@ mod tests { move |(_obj_source, _column_path_tried, _err)| ShellError::unimplemented(reason) } - fn column_path(paths: &[Value]) -> Tagged { - as_column_path(&table(paths)).unwrap() + fn column_path(paths: &[Value]) -> Result, ShellError> { + as_column_path(&table(paths)) } #[test] - fn gets_matching_field_from_a_row() { + fn gets_matching_field_from_a_row() -> Result<(), ShellError> { let row = UntaggedValue::row(indexmap! { "amigos".into() => table(&[string("andres"),string("jonathan"),string("yehuda")]) }) .into_untagged_value(); assert_eq!( - row.get_data_by_key("amigos".spanned_unknown()).unwrap(), + row.get_data_by_key("amigos".spanned_unknown()) + .ok_or_else(|| ShellError::unexpected("Failure during testing"))?, table(&[string("andres"), string("jonathan"), string("yehuda")]) ); + + Ok(()) } #[test] - fn gets_matching_field_from_nested_rows_inside_a_row() { + fn gets_matching_field_from_nested_rows_inside_a_row() -> Result<(), ShellError> { let field_path = column_path(&[string("package"), string("version")]); let (version, tag) = string("0.4.0").into_parts(); @@ -256,16 +259,19 @@ mod tests { }); assert_eq!( - *value - .into_value(tag) - .get_data_by_column_path(&field_path, Box::new(error_callback("package.version"))) - .unwrap(), + *value.into_value(tag).get_data_by_column_path( + &field_path?.item, + Box::new(error_callback("package.version")) + )?, version - ) + ); + + Ok(()) } #[test] - fn gets_first_matching_field_from_rows_with_same_field_inside_a_table() { + fn gets_first_matching_field_from_rows_with_same_field_inside_a_table() -> Result<(), ShellError> + { let field_path = column_path(&[string("package"), string("authors"), string("name")]); let (_, tag) = string("Andrés N. Robalino").into_parts(); @@ -283,23 +289,22 @@ mod tests { }); assert_eq!( - value - .into_value(tag) - .get_data_by_column_path( - &field_path, - Box::new(error_callback("package.authors.name")) - ) - .unwrap(), + value.into_value(tag).get_data_by_column_path( + &field_path?.item, + Box::new(error_callback("package.authors.name")) + )?, table(&[ string("Andrés N. Robalino"), string("Jonathan Turner"), string("Yehuda Katz") ]) - ) + ); + + Ok(()) } #[test] - fn column_path_that_contains_just_a_number_gets_a_row_from_a_table() { + fn column_path_that_contains_just_a_number_gets_a_row_from_a_table() -> Result<(), ShellError> { let field_path = column_path(&[string("package"), string("authors"), int(0)]); let (_, tag) = string("Andrés N. Robalino").into_parts(); @@ -317,18 +322,20 @@ mod tests { }); assert_eq!( - *value - .into_value(tag) - .get_data_by_column_path(&field_path, Box::new(error_callback("package.authors.0"))) - .unwrap(), + *value.into_value(tag).get_data_by_column_path( + &field_path?.item, + Box::new(error_callback("package.authors.0")) + )?, UntaggedValue::row(indexmap! { "name".into() => string("Andrés N. Robalino") }) ); + + Ok(()) } #[test] - fn column_path_that_contains_just_a_number_gets_a_row_from_a_row() { + fn column_path_that_contains_just_a_number_gets_a_row_from_a_row() -> Result<(), ShellError> { let field_path = column_path(&[string("package"), string("authors"), string("0")]); let (_, tag) = string("Andrés N. Robalino").into_parts(); @@ -346,21 +353,20 @@ mod tests { }); assert_eq!( - *value - .into_value(tag) - .get_data_by_column_path( - &field_path, - Box::new(error_callback("package.authors.\"0\"")) - ) - .unwrap(), + *value.into_value(tag).get_data_by_column_path( + &field_path?.item, + Box::new(error_callback("package.authors.\"0\"")) + )?, UntaggedValue::row(indexmap! { "name".into() => string("Andrés N. Robalino") }) ); + + Ok(()) } #[test] - fn replaces_matching_field_from_a_row() { + fn replaces_matching_field_from_a_row() -> Result<(), ShellError> { let field_path = column_path(&[string("amigos")]); let sample = UntaggedValue::row(indexmap! { @@ -375,14 +381,16 @@ mod tests { let actual = sample .into_untagged_value() - .replace_data_at_column_path(&field_path, replacement) + .replace_data_at_column_path(&field_path?.item, replacement) .unwrap(); assert_eq!(actual, row(indexmap! {"amigos".into() => string("jonas")})); + + Ok(()) } #[test] - fn replaces_matching_field_from_nested_rows_inside_a_row() { + fn replaces_matching_field_from_nested_rows_inside_a_row() -> Result<(), ShellError> { let field_path = column_path(&[ string("package"), string("authors"), @@ -404,7 +412,7 @@ mod tests { let actual = sample .into_value(tag.clone()) - .replace_data_at_column_path(&field_path, replacement.clone()) + .replace_data_at_column_path(&field_path?.item, replacement.clone()) .unwrap(); assert_eq!( @@ -417,9 +425,11 @@ mod tests { "los.3.caballeros".into() => replacement})})}) .into_value(tag) ); + + Ok(()) } #[test] - fn replaces_matching_field_from_rows_inside_a_table() { + fn replaces_matching_field_from_rows_inside_a_table() -> Result<(), ShellError> { let field_path = column_path(&[ string("shell_policy"), string("releases"), @@ -456,7 +466,7 @@ mod tests { let actual = sample .into_value(tag.clone()) - .replace_data_at_column_path(&field_path, replacement.clone()) + .replace_data_at_column_path(&field_path?.item, replacement.clone()) .unwrap(); assert_eq!( @@ -481,5 +491,7 @@ mod tests { }) }).into_value(&tag) ); + + Ok(()) } }