From aa577bf9bf306301e37b830ac637dff3bbffe58a Mon Sep 17 00:00:00 2001 From: Jonathan Turner Date: Thu, 2 Jan 2020 09:45:32 +1300 Subject: [PATCH] Clean up some unwraps (#1147) --- crates/nu-build/src/lib.rs | 17 ++--- .../src/hir/tokens_iterator/tests.rs | 8 +- crates/nu_plugin_binaryview/src/main.rs | 38 ++++------ crates/nu_plugin_fetch/src/main.rs | 26 ++++++- crates/nu_plugin_inc/src/inc.rs | 15 ++-- crates/nu_plugin_match/src/main.rs | 25 +++--- crates/nu_plugin_post/src/main.rs | 76 ++++++++++++++----- crates/nu_plugin_str/src/nu_plugin_str/mod.rs | 26 ++++++- crates/nu_plugin_str/src/strutils.rs | 42 ++++++---- src/cli.rs | 8 +- 10 files changed, 183 insertions(+), 98 deletions(-) diff --git a/crates/nu-build/src/lib.rs b/crates/nu-build/src/lib.rs index d42bb8f633..96754cce0a 100644 --- a/crates/nu-build/src/lib.rs +++ b/crates/nu-build/src/lib.rs @@ -13,10 +13,10 @@ lazy_static! { // got from https://github.com/mitsuhiko/insta/blob/b113499249584cb650150d2d01ed96ee66db6b30/src/runtime.rs#L67-L88 -fn get_cargo_workspace(manifest_dir: &str) -> Option<&Path> { - let mut workspaces = WORKSPACES.lock().unwrap(); +fn get_cargo_workspace(manifest_dir: &str) -> Result, Box> { + let mut workspaces = WORKSPACES.lock()?; if let Some(rv) = workspaces.get(manifest_dir) { - Some(rv) + Ok(Some(rv)) } else { #[derive(Deserialize)] struct Manifest { @@ -26,12 +26,11 @@ fn get_cargo_workspace(manifest_dir: &str) -> Option<&Path> { .arg("metadata") .arg("--format-version=1") .current_dir(manifest_dir) - .output() - .unwrap(); - let manifest: Manifest = serde_json::from_slice(&output.stdout).unwrap(); + .output()?; + let manifest: Manifest = serde_json::from_slice(&output.stdout)?; let path = Box::leak(Box::new(PathBuf::from(manifest.workspace_root))); workspaces.insert(manifest_dir.to_string(), path.as_path()); - workspaces.get(manifest_dir).cloned() + Ok(workspaces.get(manifest_dir).cloned()) } } @@ -43,7 +42,7 @@ struct Feature { } pub fn build() -> Result<(), Box> { - let input = env::var("CARGO_MANIFEST_DIR").unwrap(); + let input = env::var("CARGO_MANIFEST_DIR")?; let all_on = env::var("NUSHELL_ENABLE_ALL_FLAGS").is_ok(); let flags: HashSet = env::var("NUSHELL_ENABLE_FLAGS") @@ -56,7 +55,7 @@ pub fn build() -> Result<(), Box> { ); } - let workspace = match get_cargo_workspace(&input) { + let workspace = match get_cargo_workspace(&input)? { // If the crate is being downloaded from crates.io, it won't have a workspace root, and that's ok None => return Ok(()), Some(workspace) => workspace, diff --git a/crates/nu-parser/src/hir/tokens_iterator/tests.rs b/crates/nu-parser/src/hir/tokens_iterator/tests.rs index e3f44f7e05..24569a9a4b 100644 --- a/crates/nu-parser/src/hir/tokens_iterator/tests.rs +++ b/crates/nu-parser/src/hir/tokens_iterator/tests.rs @@ -3,14 +3,14 @@ use crate::parse::token_tree_builder::TokenTreeBuilder as b; use crate::Span; #[test] -fn supplies_tokens() { +fn supplies_tokens() -> Result<(), Box> { let tokens = b::token_list(vec![b::var("it"), b::op("."), b::bare("cpu")]); let (tokens, _) = b::build(tokens); let tokens = tokens.expect_list(); let mut iterator = TokensIterator::all(tokens, Span::unknown()); - iterator.next().unwrap().expect_var(); - iterator.next().unwrap().expect_dot(); - iterator.next().unwrap().expect_bare(); + iterator.next()?.expect_var(); + iterator.next()?.expect_dot(); + iterator.next()?.expect_bare(); } diff --git a/crates/nu_plugin_binaryview/src/main.rs b/crates/nu_plugin_binaryview/src/main.rs index 9d3140c299..9923a6c5ad 100644 --- a/crates/nu_plugin_binaryview/src/main.rs +++ b/crates/nu_plugin_binaryview/src/main.rs @@ -195,40 +195,32 @@ struct RawImageBuffer { buffer: Vec, } -fn load_from_png_buffer(buffer: &[u8]) -> Option { +fn load_from_png_buffer(buffer: &[u8]) -> Result> { use image::ImageDecoder; - let decoder = image::png::PNGDecoder::new(buffer); - if decoder.is_err() { - return None; - } - let decoder = decoder.unwrap(); + let decoder = image::png::PNGDecoder::new(buffer)?; let dimensions = decoder.dimensions(); let colortype = decoder.colortype(); - let buffer = decoder.read_image().unwrap(); + let buffer = decoder.read_image()?; - Some(RawImageBuffer { + Ok(RawImageBuffer { dimensions, colortype, buffer, }) } -fn load_from_jpg_buffer(buffer: &[u8]) -> Option { +fn load_from_jpg_buffer(buffer: &[u8]) -> Result> { use image::ImageDecoder; - let decoder = image::jpeg::JPEGDecoder::new(buffer); - if decoder.is_err() { - return None; - } - let decoder = decoder.unwrap(); + let decoder = image::jpeg::JPEGDecoder::new(buffer)?; let dimensions = decoder.dimensions(); let colortype = decoder.colortype(); - let buffer = decoder.read_image().unwrap(); + let buffer = decoder.read_image()?; - Some(RawImageBuffer { + Ok(RawImageBuffer { dimensions, colortype, buffer, @@ -242,16 +234,16 @@ pub fn view_contents( ) -> Result<(), Box> { let mut raw_image_buffer = load_from_png_buffer(buffer); - if raw_image_buffer.is_none() { + if raw_image_buffer.is_err() { raw_image_buffer = load_from_jpg_buffer(buffer); } - if raw_image_buffer.is_none() { + if raw_image_buffer.is_err() { //Not yet supported outln!("{:?}", buffer.hex_dump()); return Ok(()); } - let raw_image_buffer = raw_image_buffer.unwrap(); + let raw_image_buffer = raw_image_buffer?; let mut render_context: RenderContext = RenderContext::blank(lores_mode); let _ = render_context.update(); @@ -264,7 +256,7 @@ pub fn view_contents( raw_image_buffer.dimensions.1 as u32, raw_image_buffer.buffer, ) - .unwrap(); + .ok_or("Cannot convert image data")?; let resized_img = image::imageops::resize( &img, @@ -287,7 +279,7 @@ pub fn view_contents( raw_image_buffer.dimensions.1 as u32, raw_image_buffer.buffer, ) - .unwrap(); + .ok_or("Cannot convert image data")?; let resized_img = image::imageops::resize( &img, @@ -374,8 +366,8 @@ pub fn view_contents_interactive( let image_buffer = nes.image_buffer(); let slice = unsafe { std::slice::from_raw_parts(image_buffer, 256 * 240 * 4) }; - let img = - image::ImageBuffer::, &[u8]>::from_raw(256, 240, slice).unwrap(); + let img = image::ImageBuffer::, &[u8]>::from_raw(256, 240, slice) + .ok_or("Cannot convert image data")?; let resized_img = image::imageops::resize( &img, render_context.width as u32, diff --git a/crates/nu_plugin_fetch/src/main.rs b/crates/nu_plugin_fetch/src/main.rs index 20acb4253a..1e32d027ed 100644 --- a/crates/nu_plugin_fetch/src/main.rs +++ b/crates/nu_plugin_fetch/src/main.rs @@ -63,7 +63,13 @@ impl Plugin for Fetch { fn filter(&mut self, value: Value) -> Result, ShellError> { Ok(vec![block_on(fetch_helper( - &self.path.clone().unwrap(), + &self.path.clone().ok_or_else(|| { + ShellError::labeled_error( + "internal error: path not set", + "path not set", + &value.tag, + ) + })?, self.has_raw, value, ))]) @@ -93,7 +99,7 @@ async fn fetch_helper(path: &Value, has_raw: bool, row: Value) -> ReturnValue { if let Err(e) = result { return Err(e); } - let (file_extension, contents, contents_tag) = result.unwrap(); + let (file_extension, contents, contents_tag) = result?; let file_extension = if has_raw { None @@ -131,7 +137,13 @@ pub async fn fetch( match response { Ok(mut r) => match r.headers().get("content-type") { Some(content_type) => { - let content_type = Mime::from_str(content_type).unwrap(); + let content_type = Mime::from_str(content_type).map_err(|_| { + ShellError::labeled_error( + format!("MIME type unknown: {}", content_type), + "given unknown MIME type", + span, + ) + })?; match (content_type.type_(), content_type.subtype()) { (mime::APPLICATION, mime::XML) => Ok(( Some("xml".to_string()), @@ -225,7 +237,13 @@ pub async fn fetch( )), (mime::TEXT, mime::PLAIN) => { let path_extension = url::Url::parse(location) - .unwrap() + .map_err(|_| { + ShellError::labeled_error( + format!("Cannot parse URL: {}", location), + "cannot parse", + span, + ) + })? .path_segments() .and_then(|segments| segments.last()) .and_then(|name| if name.is_empty() { None } else { Some(name) }) diff --git a/crates/nu_plugin_inc/src/inc.rs b/crates/nu_plugin_inc/src/inc.rs index 888377e785..666d4f2d6b 100644 --- a/crates/nu_plugin_inc/src/inc.rs +++ b/crates/nu_plugin_inc/src/inc.rs @@ -152,24 +152,27 @@ mod tests { use nu_plugin::test_helpers::value::string; #[test] - fn major() { + fn major() -> Result<(), Box> { let mut inc = Inc::new(); inc.for_semver(SemVerAction::Major); - assert_eq!(inc.apply("0.1.3").unwrap(), string("1.0.0").value); + assert_eq!(inc.apply("0.1.3")?, string("1.0.0").value); + Ok(()) } #[test] - fn minor() { + fn minor() -> Result<(), Box> { let mut inc = Inc::new(); inc.for_semver(SemVerAction::Minor); - assert_eq!(inc.apply("0.1.3").unwrap(), string("0.2.0").value); + assert_eq!(inc.apply("0.1.3")?, string("0.2.0").value); + Ok(()) } #[test] - fn patch() { + fn patch() -> Result<(), Box> { let mut inc = Inc::new(); inc.for_semver(SemVerAction::Patch); - assert_eq!(inc.apply("0.1.3").unwrap(), string("0.1.4").value); + assert_eq!(inc.apply("0.1.3")?, string("0.1.4").value); + Ok(()) } } } diff --git a/crates/nu_plugin_match/src/main.rs b/crates/nu_plugin_match/src/main.rs index 05513c89eb..ec969dd9c3 100644 --- a/crates/nu_plugin_match/src/main.rs +++ b/crates/nu_plugin_match/src/main.rs @@ -13,11 +13,11 @@ struct Match { impl Match { #[allow(clippy::trivial_regex)] - fn new() -> Self { - Match { + fn new() -> Result> { + Ok(Match { column: String::new(), - regex: Regex::new("").unwrap(), - } + regex: Regex::new("")?, + }) } } @@ -49,14 +49,20 @@ impl Plugin for Match { match &args[1] { Value { value: UntaggedValue::Primitive(Primitive::String(s)), - .. + tag, } => { - self.regex = Regex::new(s).unwrap(); + self.regex = Regex::new(s).map_err(|_| { + ShellError::labeled_error( + "Internal error while creating regex", + "internal error created by pattern", + tag, + ) + })?; } Value { tag, .. } => { return Err(ShellError::labeled_error( "Unrecognized type in params", - "value", + "unexpected value", tag, )); } @@ -102,6 +108,7 @@ impl Plugin for Match { } } -fn main() { - serve_plugin(&mut Match::new()); +fn main() -> Result<(), Box> { + serve_plugin(&mut Match::new()?); + Ok(()) } diff --git a/crates/nu_plugin_post/src/main.rs b/crates/nu_plugin_post/src/main.rs index 43fc7037aa..4eb45ae436 100644 --- a/crates/nu_plugin_post/src/main.rs +++ b/crates/nu_plugin_post/src/main.rs @@ -26,6 +26,7 @@ struct Post { user: Option, password: Option, headers: Vec, + tag: Tag, } impl Post { @@ -37,6 +38,7 @@ impl Post { user: None, password: None, headers: vec![], + tag: Tag::unknown(), } } @@ -61,15 +63,20 @@ impl Post { file => Some(file.clone()), }; - self.user = call_info.args.get("user").map(|x| x.as_string().unwrap()); + self.user = match call_info.args.get("user") { + Some(user) => Some(user.as_string()?), + None => None, + }; - self.password = call_info - .args - .get("password") - .map(|x| x.as_string().unwrap()); + self.password = match call_info.args.get("password") { + Some(password) => Some(password.as_string()?), + None => None, + }; self.headers = get_headers(&call_info)?; + self.tag = call_info.name_tag; + ReturnSuccess::value(UntaggedValue::nothing().into_untagged_value()) } } @@ -107,9 +114,13 @@ impl Plugin for Post { fn filter(&mut self, row: Value) -> Result, ShellError> { Ok(vec![block_on(post_helper( - &self.path.clone().unwrap(), + &self.path.clone().ok_or_else(|| { + ShellError::labeled_error("expected a 'path'", "expected a 'path'", &self.tag) + })?, self.has_raw, - &self.body.clone().unwrap(), + &self.body.clone().ok_or_else(|| { + ShellError::labeled_error("expected a 'body'", "expected a 'body'", &self.tag) + })?, self.user.clone(), self.password.clone(), &self.headers.clone(), @@ -154,9 +165,7 @@ async fn post_helper( }; let (file_extension, contents, contents_tag) = - post(&path_str, &body, user, password, &headers, path_tag.clone()) - .await - .unwrap(); + post(&path_str, &body, user, password, &headers, path_tag.clone()).await?; let file_extension = if has_raw { None @@ -252,7 +261,13 @@ pub async fn post( match response { Ok(mut r) => match r.headers().get("content-type") { Some(content_type) => { - let content_type = Mime::from_str(content_type).unwrap(); + let content_type = Mime::from_str(content_type).map_err(|_| { + ShellError::labeled_error( + format!("Unknown MIME type: {}", content_type), + "unknown MIME type", + &tag, + ) + })?; match (content_type.type_(), content_type.subtype()) { (mime::APPLICATION, mime::XML) => Ok(( Some("xml".to_string()), @@ -332,7 +347,13 @@ pub async fn post( )), (mime::TEXT, mime::PLAIN) => { let path_extension = url::Url::parse(location) - .unwrap() + .map_err(|_| { + ShellError::labeled_error( + format!("could not parse URL: {}", location), + "could not parse URL", + &tag, + ) + })? .path_segments() .and_then(|segments| segments.last()) .and_then(|name| if name.is_empty() { None } else { Some(name) }) @@ -412,7 +433,13 @@ pub fn value_to_json_value(v: &Value) -> Result { serde_json::Number::from_f64( f.to_f64().expect("TODO: What about really big decimals?"), ) - .unwrap(), + .ok_or_else(|| { + ShellError::labeled_error( + "Can not convert big decimal to f64", + "cannot convert big decimal to f64", + &v.tag, + ) + })?, ), UntaggedValue::Primitive(Primitive::Int(i)) => { serde_json::Value::Number(serde_json::Number::from(CoerceInto::::coerce_into( @@ -448,13 +475,22 @@ pub fn value_to_json_value(v: &Value) -> Result { UntaggedValue::Block(_) | UntaggedValue::Primitive(Primitive::Range(_)) => { serde_json::Value::Null } - UntaggedValue::Primitive(Primitive::Binary(b)) => serde_json::Value::Array( - b.iter() - .map(|x| { - serde_json::Value::Number(serde_json::Number::from_f64(*x as f64).unwrap()) - }) - .collect(), - ), + UntaggedValue::Primitive(Primitive::Binary(b)) => { + let mut output = vec![]; + + for item in b.iter() { + output.push(serde_json::Value::Number( + serde_json::Number::from_f64(*item as f64).ok_or_else(|| { + ShellError::labeled_error( + "Cannot create number from from f64", + "cannot created number from f64", + &v.tag, + ) + })?, + )); + } + serde_json::Value::Array(output) + } UntaggedValue::Row(o) => { let mut m = serde_json::Map::new(); for (k, v) in o.entries.iter() { diff --git a/crates/nu_plugin_str/src/nu_plugin_str/mod.rs b/crates/nu_plugin_str/src/nu_plugin_str/mod.rs index bc6355ff54..bbde92604b 100644 --- a/crates/nu_plugin_str/src/nu_plugin_str/mod.rs +++ b/crates/nu_plugin_str/src/nu_plugin_str/mod.rs @@ -52,7 +52,7 @@ impl Plugin for Str { value: UntaggedValue::Primitive(Primitive::String(s)), .. } => { - self.for_substring(s.to_string()); + self.for_substring(s.to_string())?; } _ => { return Err(ShellError::labeled_error( @@ -77,12 +77,30 @@ impl Plugin for Str { if args.has("find-replace") { if let Some(Value { value: UntaggedValue::Table(arguments), - .. + tag, }) = args.get("find-replace") { self.for_replace(ReplaceAction::FindAndReplace( - arguments.get(0).unwrap().as_string()?, - arguments.get(1).unwrap().as_string()?, + arguments + .get(0) + .ok_or_else(|| { + ShellError::labeled_error( + "expected file and replace strings eg) [find replace]", + "missing find-replace values", + tag, + ) + })? + .as_string()?, + arguments + .get(1) + .ok_or_else(|| { + ShellError::labeled_error( + "expected file and replace strings eg) [find replace]", + "missing find-replace values", + tag, + ) + })? + .as_string()?, )); } } diff --git a/crates/nu_plugin_str/src/strutils.rs b/crates/nu_plugin_str/src/strutils.rs index 727ba73023..e275465c80 100644 --- a/crates/nu_plugin_str/src/strutils.rs +++ b/crates/nu_plugin_str/src/strutils.rs @@ -112,15 +112,21 @@ impl Str { } } - pub fn for_substring(&mut self, s: String) { + pub fn for_substring(&mut self, s: String) -> Result<(), ShellError> { let v: Vec<&str> = s.split(',').collect(); let start: usize = match v[0] { "" => 0, - _ => v[0].trim().parse().unwrap(), + _ => v[0] + .trim() + .parse() + .map_err(|_| ShellError::untagged_runtime_error("Could not perform substring"))?, }; let end: usize = match v[1] { "" => usize::max_value(), - _ => v[1].trim().parse().unwrap(), + _ => v[1] + .trim() + .parse() + .map_err(|_| ShellError::untagged_runtime_error("Could not perform substring"))?, }; if start > end { self.log_error("End must be greater than or equal to Start"); @@ -129,6 +135,8 @@ impl Str { } else { self.log_error("can only apply one"); } + + Ok(()) } pub fn for_replace(&mut self, mode: ReplaceAction) { @@ -206,35 +214,39 @@ pub mod tests { use nu_plugin::test_helpers::value::{int, string}; #[test] - fn downcases() { + fn downcases() -> Result<(), Box> { let mut strutils = Str::new(); strutils.for_downcase(); - assert_eq!(strutils.apply("ANDRES").unwrap(), string("andres").value); + assert_eq!(strutils.apply("ANDRES")?, string("andres").value); + Ok(()) } #[test] - fn upcases() { + fn upcases() -> Result<(), Box> { let mut strutils = Str::new(); strutils.for_upcase(); - assert_eq!(strutils.apply("andres").unwrap(), string("ANDRES").value); + assert_eq!(strutils.apply("andres")?, string("ANDRES").value); + Ok(()) } #[test] - fn converts_to_int() { + fn converts_to_int() -> Result<(), Box> { let mut strutils = Str::new(); strutils.for_to_int(); - assert_eq!(strutils.apply("9999").unwrap(), int(9999 as i64).value); + assert_eq!(strutils.apply("9999")?, int(9999 as i64).value); + Ok(()) } #[test] - fn replaces() { + fn replaces() -> Result<(), Box> { let mut strutils = Str::new(); strutils.for_replace(ReplaceAction::Direct("robalino".to_string())); - assert_eq!(strutils.apply("andres").unwrap(), string("robalino").value); + assert_eq!(strutils.apply("andres")?, string("robalino").value); + Ok(()) } #[test] - fn find_and_replaces() { + fn find_and_replaces() -> Result<(), Box> { let mut strutils = Str::new(); strutils.for_replace(ReplaceAction::FindAndReplace( @@ -242,9 +254,7 @@ pub mod tests { "jotandrehuda".to_string(), )); - assert_eq!( - strutils.apply("wykittens").unwrap(), - string("wyjotandrehuda").value - ); + assert_eq!(strutils.apply("wykittens")?, string("wyjotandrehuda").value); + Ok(()) } } diff --git a/src/cli.rs b/src/cli.rs index c5a23684e4..874c045f1f 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -422,9 +422,11 @@ pub async fn cli() -> Result<(), Box> { }; let prompt = { - let bytes = strip_ansi_escapes::strip(&colored_prompt).unwrap(); - - String::from_utf8_lossy(&bytes).to_string() + if let Ok(bytes) = strip_ansi_escapes::strip(&colored_prompt) { + String::from_utf8_lossy(&bytes).to_string() + } else { + "> ".to_string() + } }; rl.helper_mut().expect("No helper").colored_prompt = colored_prompt;