From 5dd20850b530d5d4943af6d0189b42b29ed2f3a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20N=2E=20Robalino?= Date: Sun, 18 Aug 2019 19:12:28 -0500 Subject: [PATCH] Refactoring and unwrap cleanup beginnings. --- src/commands/cp.rs | 4 +- src/commands/mv.rs | 230 +++++++++++++++++++++++++++---------------- src/plugin.rs | 19 +++- src/plugins/inc.rs | 37 ++++--- src/plugins/str.rs | 36 +++---- src/utils.rs | 32 +++--- tests/helpers/mod.rs | 24 +++-- 7 files changed, 236 insertions(+), 146 deletions(-) diff --git a/src/commands/cp.rs b/src/commands/cp.rs index 5fe3613ac..a41f4c0bc 100644 --- a/src/commands/cp.rs +++ b/src/commands/cp.rs @@ -21,7 +21,7 @@ impl PerItemCommand for Cpy { call_info: &CallInfo, _registry: &CommandRegistry, shell_manager: &ShellManager, - input: Tagged, + _input: Tagged, ) -> Result, ShellError> { call_info.process(shell_manager, cp)?.run() } @@ -75,7 +75,7 @@ pub fn cp( let mut sources: FileStructure = FileStructure::new(); - sources.walk_decorate(&entry); + sources.walk_decorate(&entry)?; if entry.is_file() { let strategy = |(source_file, _depth_level)| { diff --git a/src/commands/mv.rs b/src/commands/mv.rs index 54a0192bc..5b4af1612 100644 --- a/src/commands/mv.rs +++ b/src/commands/mv.rs @@ -1,3 +1,4 @@ +use crate::commands::command::RunnablePerItemContext; use crate::errors::ShellError; use crate::parser::hir::SyntaxType; use crate::parser::registry::{CommandRegistry, Signature}; @@ -9,6 +10,12 @@ use crate::utils::FileStructure; pub struct Move; +#[derive(Deserialize)] +pub struct MoveArgs { + source: Tagged, + destination: Tagged, +} + impl PerItemCommand for Move { fn run( &self, @@ -17,7 +24,7 @@ impl PerItemCommand for Move { shell_manager: &ShellManager, _input: Tagged, ) -> Result, ShellError> { - mv(call_info, shell_manager) + call_info.process(shell_manager, mv)?.run() } fn name(&self) -> &str { @@ -25,59 +32,77 @@ impl PerItemCommand for Move { } fn signature(&self) -> Signature { - Signature::build("mv").named("file", SyntaxType::Any) + Signature::build("mv") + .required("source", SyntaxType::Path) + .required("destination", SyntaxType::Path) + .named("file", SyntaxType::Any) } } pub fn mv( - call_info: &CallInfo, - shell_manager: &ShellManager, + args: MoveArgs, + context: &RunnablePerItemContext, ) -> Result, ShellError> { - let mut source = PathBuf::from(shell_manager.path()); - let mut destination = PathBuf::from(shell_manager.path()); - let span = call_info.name_span; + let mut source = PathBuf::from(context.shell_manager.path()); + let mut destination = PathBuf::from(context.shell_manager.path()); + let name_span = context.name; - match call_info - .args - .nth(0) - .ok_or_else(|| ShellError::string(&format!("No file or directory specified")))? - .as_string()? - .as_str() - { - file => { - source.push(file); + source.push(&args.source.item); + + destination.push(&args.destination.item); + + let sources: Vec<_> = match glob::glob(&source.to_string_lossy()) { + Ok(files) => files.collect(), + Err(_) => { + return Err(ShellError::labeled_error( + "Invalid pattern.", + "Invalid pattern.", + args.source.tag, + )) } - } + }; - match call_info - .args - .nth(1) - .ok_or_else(|| ShellError::string(&format!("No file or directory specified")))? - .as_string()? - .as_str() - { - file => { - destination.push(file); + let destination_file_name = { + let path = &destination; + + match path.file_name() { + Some(name) => PathBuf::from(name), + None => { + return Err(ShellError::labeled_error( + "Rename aborted. Not a valid destination", + "Rename aborted. Not a valid destination", + name_span, + )) + } } - } - - let sources = glob::glob(&source.to_string_lossy()); - - if sources.is_err() { - return Err(ShellError::labeled_error( - "Invalid pattern.", - "Invalid pattern.", - call_info.args.nth(0).unwrap().span(), - )); - } - - let sources: Vec<_> = sources.unwrap().collect(); + }; if sources.len() == 1 { if let Ok(entry) = &sources[0] { + let entry_file_name = match entry.file_name() { + Some(name) => name, + None => { + return Err(ShellError::labeled_error( + "Rename aborted. Not a valid entry name", + "Rename aborted. Not a valid entry name", + name_span, + )) + } + }; + if destination.exists() && destination.is_dir() { - destination = dunce::canonicalize(&destination).unwrap(); - destination.push(source.file_name().unwrap()); + destination = match dunce::canonicalize(&destination) { + Ok(path) => path, + Err(e) => { + return Err(ShellError::labeled_error( + format!("Rename aborted. {:}", e.to_string()), + format!("Rename aborted. {:}", e.to_string()), + name_span, + )) + } + }; + + destination.push(entry_file_name); } if entry.is_file() { @@ -86,17 +111,17 @@ pub fn mv( return Err(ShellError::labeled_error( format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), - span, + name_span, )); } Ok(o) => o, @@ -109,17 +134,17 @@ pub fn mv( return Err(ShellError::labeled_error( format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), - span, + name_span, )); } Ok(o) => o, @@ -131,17 +156,17 @@ pub fn mv( return Err(ShellError::labeled_error( format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), - span, + name_span, )); } Ok(o) => o, @@ -151,10 +176,11 @@ pub fn mv( { let mut sources: FileStructure = FileStructure::new(); - sources.walk_decorate(&entry); + sources.walk_decorate(&entry)?; let strategy = |(source_file, depth_level)| { let mut new_dst = destination.clone(); + let path = dunce::canonicalize(&source_file).unwrap(); let mut comps: Vec<_> = path @@ -181,21 +207,21 @@ pub fn mv( return Err(ShellError::labeled_error( format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), - span, + name_span, )); } Ok(o) => o, - }; + } } } @@ -205,42 +231,78 @@ pub fn mv( return Err(ShellError::labeled_error( format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), - span, + name_span, )); } Ok(o) => o, - }; + } } } - std::fs::remove_dir_all(entry).expect("can not remove directory"); + match std::fs::remove_dir_all(entry) { + Err(e) => { + return Err(ShellError::labeled_error( + format!( + "Rename {:?} to {:?} aborted. {:}", + entry_file_name, + destination_file_name, + e.to_string(), + ), + format!( + "Rename {:?} to {:?} aborted. {:}", + entry_file_name, + destination_file_name, + e.to_string(), + ), + name_span, + )); + } + Ok(o) => o, + }; } } } } else { if destination.exists() { - if !sources.iter().all(|x| (x.as_ref().unwrap()).is_file()) { + if !sources.iter().all(|x| { + if let Ok(entry) = x.as_ref() { + entry.is_file() + } else { + false + } + }) { return Err(ShellError::labeled_error( "Rename aborted (directories found). Renaming in patterns not supported yet (try moving the directory directly)", "Rename aborted (directories found). Renaming in patterns not supported yet (try moving the directory directly)", - call_info.args.nth(0).unwrap().span(), + args.source.tag, )); } for entry in sources { if let Ok(entry) = entry { + let entry_file_name = match entry.file_name() { + Some(name) => name, + None => { + return Err(ShellError::labeled_error( + "Rename aborted. Not a valid entry name", + "Rename aborted. Not a valid entry name", + name_span, + )) + } + }; + let mut to = PathBuf::from(&destination); - to.push(&entry.file_name().unwrap()); + to.push(entry_file_name); if entry.is_file() { match std::fs::rename(&entry, &to) { @@ -248,17 +310,17 @@ pub fn mv( return Err(ShellError::labeled_error( format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), format!( "Rename {:?} to {:?} aborted. {:}", - entry.file_name().unwrap(), - destination.file_name().unwrap(), + entry_file_name, + destination_file_name, e.to_string(), ), - span, + name_span, )); } Ok(o) => o, @@ -268,15 +330,9 @@ pub fn mv( } } else { return Err(ShellError::labeled_error( - format!( - "Rename aborted. (Does {:?} exist?)", - &destination.file_name().unwrap() - ), - format!( - "Rename aborted. (Does {:?} exist?)", - &destination.file_name().unwrap() - ), - call_info.args.nth(1).unwrap().span(), + format!("Rename aborted. (Does {:?} exist?)", destination_file_name), + format!("Rename aborted. (Does {:?} exist?)", destination_file_name), + args.destination.span(), )); } } diff --git a/src/plugin.rs b/src/plugin.rs index 7ead3c9de..469413ba5 100644 --- a/src/plugin.rs +++ b/src/plugin.rs @@ -28,7 +28,16 @@ pub trait Plugin { pub fn serve_plugin(plugin: &mut dyn Plugin) { let args = std::env::args(); if args.len() > 1 { - let input = std::fs::read_to_string(args.skip(1).next().unwrap()); + let input = args.skip(1).next(); + + let input = match input { + Some(arg) => std::fs::read_to_string(arg), + None => { + send_response(ShellError::string(format!("No input given."))); + return; + } + }; + if let Ok(input) = input { let command = serde_json::from_str::(&input); match command { @@ -128,8 +137,12 @@ impl JsonRpc { fn send_response(result: T) { let response = JsonRpc::new("response", result); - let response_raw = serde_json::to_string(&response).unwrap(); - println!("{}", response_raw); + let response_raw = serde_json::to_string(&response); + + match response_raw { + Ok(response) => println!("{}", response), + Err(err) => println!("{}", err), + } } #[derive(Debug, Serialize, Deserialize)] #[serde(tag = "method")] diff --git a/src/plugins/inc.rs b/src/plugins/inc.rs index cde6b15ab..173ced2fe 100644 --- a/src/plugins/inc.rs +++ b/src/plugins/inc.rs @@ -1,7 +1,7 @@ use indexmap::IndexMap; use nu::{ - serve_plugin, CallInfo, NamedType, Plugin, Primitive, ReturnSuccess, - ReturnValue, ShellError, Signature, Tagged, TaggedItem, Value, + serve_plugin, CallInfo, NamedType, Plugin, Primitive, ReturnSuccess, ReturnValue, ShellError, + Signature, Tagged, TaggedItem, Value, }; enum Action { @@ -30,16 +30,13 @@ impl Inc { } } - fn apply(&self, input: &str) -> Value { - match &self.action { + fn apply(&self, input: &str) -> Result { + let applied = match &self.action { Some(Action::SemVerAction(act_on)) => { - let ver = semver::Version::parse(&input); - - if ver.is_err() { - return Value::string(input.to_string()); - } - - let mut ver = ver.unwrap(); + let mut ver = match semver::Version::parse(&input) { + Ok(parsed_ver) => parsed_ver, + Err(_) => return Ok(Value::string(input.to_string())), + }; match act_on { SemVerAction::Major => ver.increment_major(), @@ -53,7 +50,9 @@ impl Inc { Ok(v) => Value::string(format!("{}", v + 1)), Err(_) => Value::string(input), }, - } + }; + + Ok(applied) } fn for_semver(&mut self, part: SemVerAction) { @@ -83,7 +82,7 @@ impl Inc { Ok(Value::bytes(b + 1 as u64).tagged(value.tag())) } Value::Primitive(Primitive::String(ref s)) => { - Ok(Tagged::from_item(self.apply(&s), value.tag())) + Ok(Tagged::from_item(self.apply(&s)?, value.tag())) } Value::Object(_) => match self.field { Some(ref f) => { @@ -187,8 +186,8 @@ mod tests { use super::{Inc, SemVerAction}; use indexmap::IndexMap; use nu::{ - CallInfo, EvaluatedArgs, Plugin, ReturnSuccess, SourceMap, Span, Tag, Tagged, TaggedDictBuilder, - TaggedItem, Value, + CallInfo, EvaluatedArgs, Plugin, ReturnSuccess, SourceMap, Span, Tag, Tagged, + TaggedDictBuilder, TaggedItem, Value, }; struct CallStub { @@ -237,7 +236,7 @@ mod tests { fn inc_plugin_configuration_flags_wired() { let mut plugin = Inc::new(); - let configured = plugin.config().unwrap(); + let configured = plugin.config().expect("Can not configure plugin"); for action_flag in &["major", "minor", "patch"] { assert!(configured.named.get(*action_flag).is_some()); @@ -304,21 +303,21 @@ mod tests { fn incs_major() { let mut inc = Inc::new(); inc.for_semver(SemVerAction::Major); - assert_eq!(inc.apply("0.1.3"), Value::string("1.0.0")); + assert_eq!(inc.apply("0.1.3").unwrap(), Value::string("1.0.0")); } #[test] fn incs_minor() { let mut inc = Inc::new(); inc.for_semver(SemVerAction::Minor); - assert_eq!(inc.apply("0.1.3"), Value::string("0.2.0")); + assert_eq!(inc.apply("0.1.3").unwrap(), Value::string("0.2.0")); } #[test] fn incs_patch() { let mut inc = Inc::new(); inc.for_semver(SemVerAction::Patch); - assert_eq!(inc.apply("0.1.3"), Value::string("0.1.4")); + assert_eq!(inc.apply("0.1.3").unwrap(), Value::string("0.1.4")); } #[test] diff --git a/src/plugins/str.rs b/src/plugins/str.rs index 3c9201174..679ede80a 100644 --- a/src/plugins/str.rs +++ b/src/plugins/str.rs @@ -36,19 +36,15 @@ impl Str { } } - fn apply(&self, input: &str) -> Value { - if self.action.is_none() { - return Value::string(input.to_string()); - } - - match self.action.as_ref().unwrap() { - Action::Downcase => Value::string(input.to_ascii_lowercase()), - Action::Upcase => Value::string(input.to_ascii_uppercase()), - Action::ToInteger => match input.trim().parse::() { + fn apply(&self, input: &str) -> Result { + let applied = match self.action.as_ref() { + Some(Action::Downcase) => Value::string(input.to_ascii_lowercase()), + Some(Action::Upcase) => Value::string(input.to_ascii_uppercase()), + Some(Action::ToInteger) => match input.trim().parse::() { Ok(v) => Value::int(v), Err(_) => Value::string(input), }, - Action::Replace(ref mode) => match mode { + Some(Action::Replace(ref mode)) => match mode { ReplaceAction::Direct => Value::string(self.first_param()), ReplaceAction::FindAndReplace => { let regex = Regex::new(self.first_param()); @@ -59,7 +55,10 @@ impl Str { } } }, - } + None => Value::string(input), + }; + + Ok(applied) } fn did_supply_field(&self) -> bool { @@ -133,7 +132,7 @@ impl Str { fn strutils(&self, value: Tagged) -> Result, ShellError> { match value.item { Value::Primitive(Primitive::String(ref s)) => { - Ok(Tagged::from_item(self.apply(&s), value.tag())) + Ok(Tagged::from_item(self.apply(&s)?, value.tag())) } Value::Object(_) => match self.field { Some(ref f) => { @@ -431,21 +430,21 @@ mod tests { fn str_downcases() { let mut strutils = Str::new(); strutils.for_downcase(); - assert_eq!(strutils.apply("ANDRES"), Value::string("andres")); + assert_eq!(strutils.apply("ANDRES").unwrap(), Value::string("andres")); } #[test] fn str_upcases() { let mut strutils = Str::new(); strutils.for_upcase(); - assert_eq!(strutils.apply("andres"), Value::string("ANDRES")); + assert_eq!(strutils.apply("andres").unwrap(), Value::string("ANDRES")); } #[test] fn str_to_int() { let mut strutils = Str::new(); strutils.for_to_int(); - assert_eq!(strutils.apply("9999"), Value::int(9999 as i64)); + assert_eq!(strutils.apply("9999").unwrap(), Value::int(9999 as i64)); } #[test] @@ -453,7 +452,7 @@ mod tests { let mut strutils = Str::new(); strutils.for_replace(ReplaceAction::Direct); strutils.replace_with("robalino"); - assert_eq!(strutils.apply("andres"), Value::string("robalino")); + assert_eq!(strutils.apply("andres").unwrap(), Value::string("robalino")); } #[test] @@ -462,7 +461,10 @@ mod tests { strutils.for_replace(ReplaceAction::FindAndReplace); strutils.find_with(r"kittens"); strutils.replace_with("jotandrehuda"); - assert_eq!(strutils.apply("wykittens"), Value::string("wyjotandrehuda")); + assert_eq!( + strutils.apply("wykittens").unwrap(), + Value::string("wyjotandrehuda") + ); } #[test] diff --git a/src/utils.rs b/src/utils.rs index 95642d1d7..ac17462f3 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,3 +1,4 @@ +use crate::errors::ShellError; use std::ops::Div; use std::path::{Path, PathBuf}; @@ -71,7 +72,6 @@ impl> Div for &RelativePath { } } - #[derive(Debug, Eq, Ord, PartialEq, PartialOrd)] pub struct Res { pub loc: PathBuf, @@ -108,23 +108,25 @@ impl FileStructure { .collect() } - pub fn walk_decorate(&mut self, start_path: &Path) { - self.set_root(&dunce::canonicalize(start_path).unwrap()); + pub fn walk_decorate(&mut self, start_path: &Path) -> Result<(), ShellError> { + self.set_root(&dunce::canonicalize(start_path)?); self.resources = Vec::::new(); - self.build(start_path, 0); + self.build(start_path, 0)?; self.resources.sort(); + + Ok(()) } - fn build(&mut self, src: &'a Path, lvl: usize) { - let source = dunce::canonicalize(src).unwrap(); + fn build(&mut self, src: &'a Path, lvl: usize) -> Result<(), ShellError> { + let source = dunce::canonicalize(src)?; if source.is_dir() { - for entry in std::fs::read_dir(&source).unwrap() { - let entry = entry.unwrap(); + for entry in std::fs::read_dir(&source)? { + let entry = entry?; let path = entry.path(); if path.is_dir() { - self.build(&path, lvl + 1); + self.build(&path, lvl + 1)?; } self.resources.push(Res { @@ -138,6 +140,8 @@ impl FileStructure { at: lvl, }); } + + Ok(()) } } @@ -152,13 +156,18 @@ mod tests { sdx.push("tests"); sdx.push("fixtures"); sdx.push("formats"); - dunce::canonicalize(sdx).unwrap() + + match dunce::canonicalize(sdx) { + Ok(path) => path, + Err(_) => panic!("Wrong path."), + } } #[test] fn prepares_and_decorates_source_files_for_copying() { let mut res = FileStructure::new(); - res.walk_decorate(fixtures().as_path()); + + res.walk_decorate(fixtures().as_path()).expect("Can not decorate files traversal."); assert_eq!( res.resources, @@ -195,4 +204,3 @@ mod tests { ); } } - diff --git a/tests/helpers/mod.rs b/tests/helpers/mod.rs index 0df348f94..67c788380 100644 --- a/tests/helpers/mod.rs +++ b/tests/helpers/mod.rs @@ -151,7 +151,20 @@ impl Playground { } pub fn glob_vec(pattern: &str) -> Vec { - glob(pattern).unwrap().map(|r| r.unwrap()).collect() + let glob = glob(pattern); + + match glob { + Ok(paths) => paths + .map(|path| { + if let Ok(path) = path { + path + } else { + unreachable!() + } + }) + .collect(), + Err(_) => panic!("Invalid pattern."), + } } } @@ -178,11 +191,10 @@ pub fn normalize_string(input: &str) -> String { pub fn create_file_at(full_path: impl AsRef) -> Result<(), std::io::Error> { let full_path = full_path.as_ref(); - assert!( - full_path.parent().unwrap().is_dir(), - "{:?} exists", - full_path.parent().unwrap().display(), - ); + if let Some(parent) = full_path.parent() { + panic!(format!("{:?} exists", parent.display())); + } + std::fs::write(full_path, "fake data".as_bytes()) }