From 8d5fd6f3790997b55413eafa6a54ab8b5376f008 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20N=2E=20Robalino?= Date: Wed, 21 Aug 2019 07:08:23 -0500 Subject: [PATCH] Unwrap cleanup mitigation. --- src/commands/command.rs | 1 + src/commands/cp.rs | 110 +++++++++++------- src/commands/mv.rs | 44 ++++---- src/commands/pick.rs | 16 +-- src/commands/rm.rs | 42 +++---- src/commands/save.rs | 242 ++++++++++++++++++++-------------------- src/errors.rs | 21 ++++ 7 files changed, 255 insertions(+), 221 deletions(-) diff --git a/src/commands/command.rs b/src/commands/command.rs index 19457f3db..76a13e407 100644 --- a/src/commands/command.rs +++ b/src/commands/command.rs @@ -238,6 +238,7 @@ pub struct RunnableContext { } impl RunnableContext { + #[allow(unused)] pub fn cwd(&self) -> PathBuf { PathBuf::from(self.shell_manager.path()) } diff --git a/src/commands/cp.rs b/src/commands/cp.rs index a41f4c0bc..a6ba7ecd0 100644 --- a/src/commands/cp.rs +++ b/src/commands/cp.rs @@ -10,8 +10,8 @@ pub struct Cpy; #[derive(Deserialize)] pub struct CopyArgs { - source: Tagged, - destination: Tagged, + src: Tagged, + dst: Tagged, recursive: Tagged, } @@ -32,44 +32,43 @@ impl PerItemCommand for Cpy { fn signature(&self) -> Signature { Signature::build("cp") - .required("source", SyntaxType::Path) - .required("destination", SyntaxType::Path) + .required("src", SyntaxType::Path) + .required("dst", SyntaxType::Path) .named("file", SyntaxType::Any) .switch("recursive") } } -pub fn cp( - args: CopyArgs, - context: &RunnablePerItemContext, +fn cp( + CopyArgs { + src, + dst, + recursive, + }: CopyArgs, + RunnablePerItemContext { name, .. }: &RunnablePerItemContext, ) -> Result, ShellError> { - let mut source = PathBuf::from(context.shell_manager.path()); - let mut destination = PathBuf::from(context.shell_manager.path()); - let name_span = context.name; + let source = src.item.clone(); + let mut destination = dst.item.clone(); + let name_span = name; - source.push(&args.source.item); - - destination.push(&args.destination.item); - - let sources = glob::glob(&source.to_string_lossy()); - - if sources.is_err() { - return Err(ShellError::labeled_error( - "Invalid pattern.", - "Invalid pattern.", - args.source.tag, - )); - } - - let sources: Vec<_> = sources.unwrap().collect(); + let sources: Vec<_> = match glob::glob(&source.to_string_lossy()) { + Ok(files) => files.collect(), + Err(_) => { + return Err(ShellError::labeled_error( + "Invalid pattern.", + "Invalid pattern.", + src.tag, + )) + } + }; if sources.len() == 1 { if let Ok(entry) = &sources[0] { - if entry.is_dir() && !args.recursive.item { + if entry.is_dir() && !recursive.item { return Err(ShellError::labeled_error( "is a directory (not copied). Try using \"--recursive\".", "is a directory (not copied). Try using \"--recursive\".", - args.source.tag, + src.tag, )); } @@ -167,7 +166,16 @@ pub fn cp( } } } else { - destination.push(entry.file_name().unwrap()); + match entry.file_name() { + Some(name) => destination.push(name), + None => { + return Err(ShellError::labeled_error( + "Copy aborted. Not a valid path", + "Copy aborted. Not a valid path", + name_span, + )) + } + } match std::fs::create_dir_all(&destination) { Err(e) => { @@ -234,18 +242,31 @@ pub fn cp( } } else { if destination.exists() { - if !sources.iter().all(|x| (x.as_ref().unwrap()).is_file()) { + if !sources.iter().all(|x| match x { + Ok(f) => f.is_file(), + Err(_) => false, + }) { return Err(ShellError::labeled_error( "Copy aborted (directories found). Recursive copying in patterns not supported yet (try copying the directory directly)", "Copy aborted (directories found). Recursive copying in patterns not supported yet (try copying the directory directly)", - args.source.tag, + src.tag, )); } for entry in sources { if let Ok(entry) = entry { let mut to = PathBuf::from(&destination); - to.push(&entry.file_name().unwrap()); + + match entry.file_name() { + Some(name) => to.push(name), + None => { + return Err(ShellError::labeled_error( + "Copy aborted. Not a valid path", + "Copy aborted. Not a valid path", + name_span, + )) + } + } if entry.is_file() { match std::fs::copy(&entry, &to) { @@ -253,7 +274,7 @@ pub fn cp( return Err(ShellError::labeled_error( e.to_string(), e.to_string(), - args.source.tag, + src.tag, )); } Ok(o) => o, @@ -262,16 +283,23 @@ pub fn cp( } } } else { + let destination_file_name = { + match destination.file_name() { + Some(name) => PathBuf::from(name), + None => { + return Err(ShellError::labeled_error( + "Copy aborted. Not a valid destination", + "Copy aborted. Not a valid destination", + name_span, + )) + } + } + }; + return Err(ShellError::labeled_error( - format!( - "Copy aborted. (Does {:?} exist?)", - &destination.file_name().unwrap() - ), - format!( - "Copy aborted. (Does {:?} exist?)", - &destination.file_name().unwrap() - ), - args.destination.span(), + format!("Copy aborted. (Does {:?} exist?)", destination_file_name), + format!("Copy aborted. (Does {:?} exist?)", destination_file_name), + &dst.span(), )); } } diff --git a/src/commands/mv.rs b/src/commands/mv.rs index a660b6fb6..8f78e2d26 100644 --- a/src/commands/mv.rs +++ b/src/commands/mv.rs @@ -5,15 +5,12 @@ use crate::parser::registry::{CommandRegistry, Signature}; use crate::prelude::*; use std::path::PathBuf; -#[cfg(windows)] -use crate::utils::FileStructure; - pub struct Move; #[derive(Deserialize)] pub struct MoveArgs { - source: Tagged, - destination: Tagged, + src: Tagged, + dst: Tagged, } impl PerItemCommand for Move { @@ -39,17 +36,16 @@ impl PerItemCommand for Move { } } -pub fn mv( - args: MoveArgs, - context: &RunnablePerItemContext, +fn mv( + MoveArgs { src, dst }: MoveArgs, + RunnablePerItemContext { + name, + shell_manager, + }: &RunnablePerItemContext, ) -> Result, ShellError> { - let mut source = PathBuf::from(context.shell_manager.path()); - let mut destination = PathBuf::from(context.shell_manager.path()); - let name_span = context.name; - - source.push(&args.source.item); - - destination.push(&args.destination.item); + let source = src.item.clone(); + let mut destination = dst.item.clone(); + let name_span = name; let sources: Vec<_> = match glob::glob(&source.to_string_lossy()) { Ok(files) => files.collect(), @@ -57,21 +53,23 @@ pub fn mv( return Err(ShellError::labeled_error( "Invalid pattern.", "Invalid pattern.", - args.source.tag, + src.tag, )) } }; - let destination_file_name = { - let path = &destination; + if "." == destination.to_string_lossy() { + destination = PathBuf::from(shell_manager.path()); + } - match path.file_name() { + let destination_file_name = { + match destination.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, + dst.span(), )) } } @@ -174,6 +172,8 @@ pub fn mv( } #[cfg(windows)] { + use crate::utils::FileStructure; + let mut sources: FileStructure = FileStructure::new(); sources.walk_decorate(&entry)?; @@ -284,7 +284,7 @@ pub fn mv( 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)", - args.source.tag, + src.tag, )); } @@ -332,7 +332,7 @@ pub fn mv( return Err(ShellError::labeled_error( format!("Rename aborted. (Does {:?} exist?)", destination_file_name), format!("Rename aborted. (Does {:?} exist?)", destination_file_name), - args.destination.span(), + dst.span(), )); } } diff --git a/src/commands/pick.rs b/src/commands/pick.rs index 21a83480f..dbebff063 100644 --- a/src/commands/pick.rs +++ b/src/commands/pick.rs @@ -12,14 +12,6 @@ struct PickArgs { pub struct Pick; impl WholeStreamCommand for Pick { - fn run( - &self, - args: CommandArgs, - registry: &CommandRegistry, - ) -> Result { - args.process(registry, pick)?.run() - } - fn name(&self) -> &str { "pick" } @@ -27,6 +19,14 @@ impl WholeStreamCommand for Pick { fn signature(&self) -> Signature { Signature::build("pick").rest() } + + fn run( + &self, + args: CommandArgs, + registry: &CommandRegistry, + ) -> Result { + args.process(registry, pick)?.run() + } } fn pick( diff --git a/src/commands/rm.rs b/src/commands/rm.rs index 714fb4799..9493bd94d 100644 --- a/src/commands/rm.rs +++ b/src/commands/rm.rs @@ -10,7 +10,7 @@ pub struct Remove; #[derive(Deserialize)] pub struct RemoveArgs { - path: Tagged, + target: Tagged, recursive: Tagged, } @@ -36,32 +36,30 @@ impl PerItemCommand for Remove { } } -pub fn rm( - args: RemoveArgs, - context: &RunnablePerItemContext, +fn rm( + RemoveArgs { target, recursive }: RemoveArgs, + RunnablePerItemContext { name, .. }: &RunnablePerItemContext, ) -> Result, ShellError> { - let mut path = PathBuf::from(context.shell_manager.path()); - let name_span = context.name; + let path = target.item.clone(); + let name_span = name; - let file = &args.path.item.to_string_lossy(); + let file = path.to_string_lossy(); if file == "." || file == ".." { return Err(ShellError::labeled_error( "Remove aborted. \".\" or \"..\" may not be removed.", "Remove aborted. \".\" or \"..\" may not be removed.", - args.path.span(), + target.span(), )); } - path.push(&args.path.item); - let entries: Vec<_> = match glob::glob(&path.to_string_lossy()) { Ok(files) => files.collect(), Err(_) => { return Err(ShellError::labeled_error( "Invalid pattern.", "Invalid pattern.", - args.path.tag, + target.tag, )) } }; @@ -73,17 +71,11 @@ pub fn rm( source_dir.walk_decorate(&entry)?; - if source_dir.contains_files() && !args.recursive.item { + if source_dir.contains_files() && !recursive.item { return Err(ShellError::labeled_error( - format!( - "{:?} is a directory. Try using \"--recursive\".", - &args.path.item.to_string_lossy() - ), - format!( - "{:?} is a directory. Try using \"--recursive\".", - &args.path.item.to_string_lossy() - ), - args.path.span(), + format!("{:?} is a directory. Try using \"--recursive\".", file), + format!("{:?} is a directory. Try using \"--recursive\".", file), + target.span(), )); } } @@ -94,9 +86,7 @@ pub fn rm( match entry { Ok(path) => { let path_file_name = { - let p = &path; - - match p.file_name() { + match path.file_name() { Some(name) => PathBuf::from(name), None => { return Err(ShellError::labeled_error( @@ -112,7 +102,7 @@ pub fn rm( source_dir.walk_decorate(&path)?; - if source_dir.contains_more_than_one_file() && !args.recursive.item { + if source_dir.contains_more_than_one_file() && !recursive.item { return Err(ShellError::labeled_error( format!( "Directory {:?} found somewhere inside. Try using \"--recursive\".", @@ -122,7 +112,7 @@ pub fn rm( "Directory {:?} found somewhere inside. Try using \"--recursive\".", path_file_name ), - args.path.span(), + target.span(), )); } diff --git a/src/commands/save.rs b/src/commands/save.rs index b0c809a33..37882f429 100644 --- a/src/commands/save.rs +++ b/src/commands/save.rs @@ -36,19 +36,26 @@ impl WholeStreamCommand for Save { } } -pub fn save( +fn save( SaveArgs { path, raw: save_raw, }: SaveArgs, - context: RunnableContext, + RunnableContext { + input, + name, + shell_manager, + source_map, + .. + }: RunnableContext, ) -> Result { - let mut full_path = context.cwd(); + let mut full_path = PathBuf::from(shell_manager.path()); + let name_span = name; if path.is_none() { - let source_map = context.source_map.clone(); + let source_map = source_map.clone(); let stream = async_stream_block! { - let input: Vec> = context.input.values.collect().await; + let input: Vec> = input.values.collect().await; // If there is no filename, check the metadata for the origin filename if input.len() > 0 { let origin = input[0].origin(); @@ -61,7 +68,7 @@ pub fn save( yield Err(ShellError::labeled_error( "Save requires a filepath", "needs path", - context.name, + name_span, )); } }, @@ -69,7 +76,7 @@ pub fn save( yield Err(ShellError::labeled_error( "Save requires a filepath", "needs path", - context.name, + name_span, )); } } @@ -77,139 +84,126 @@ pub fn save( yield Err(ShellError::labeled_error( "Save requires a filepath", "needs path", - context.name, + name_span, )); } - let contents = match full_path.extension() { - Some(x) if x == "csv" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to csv requires a single object (or use --raw)", - )); - } - to_csv_to_string(&value_to_csv_value(&input[0])).unwrap() - } - Some(x) if x == "toml" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to toml requires a single object (or use --raw)", - )); - } - toml::to_string(&value_to_toml_value(&input[0])).unwrap() - } - Some(x) if x == "json" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to json requires a single object (or use --raw)", - )); - } - serde_json::to_string(&value_to_json_value(&input[0])).unwrap() - } - Some(x) if x == "yml" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to yml requires a single object (or use --raw)", - )); - } - serde_yaml::to_string(&value_to_yaml_value(&input[0])).unwrap() - } - Some(x) if x == "yaml" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to yaml requires a single object (or use --raw)", - )); - } - serde_yaml::to_string(&value_to_yaml_value(&input[0])).unwrap() - } - _ => { - let mut save_data = String::new(); - if input.len() > 0 { - let mut first = true; - for i in input.iter() { - if !first { - save_data.push_str("\n"); - } else { - first = false; - } - save_data.push_str(&i.as_string().unwrap()); - } - } - save_data - } + let content = if !save_raw { + to_string_for(full_path.extension(), &input) + } else { + string_from(&input) }; - let _ = std::fs::write(full_path, contents); + match content { + Ok(save_data) => match std::fs::write(full_path, save_data) { + Ok(o) => o, + Err(e) => yield Err(ShellError::string(e.to_string())), + }, + Err(e) => yield Err(ShellError::string(e.to_string())), + } + }; Ok(OutputStream::new(stream)) } else { - full_path.push(path.unwrap().item()); + if let Some(file) = path { + full_path.push(file.item()); + } let stream = async_stream_block! { - let input: Vec> = context.input.values.collect().await; + let input: Vec> = input.values.collect().await; - let contents = match full_path.extension() { - Some(x) if x == "csv" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to csv requires a single object (or use --raw)", - )); - } - to_csv_to_string(&value_to_csv_value(&input[0])).unwrap() - } - Some(x) if x == "toml" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to toml requires a single object (or use --raw)", - )); - } - toml::to_string(&value_to_toml_value(&input[0])).unwrap() - } - Some(x) if x == "json" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to json requires a single object (or use --raw)", - )); - } - serde_json::to_string(&value_to_json_value(&input[0])).unwrap() - } - Some(x) if x == "yml" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to yml requires a single object (or use --raw)", - )); - } - serde_yaml::to_string(&value_to_yaml_value(&input[0])).unwrap() - } - Some(x) if x == "yaml" && !save_raw => { - if input.len() != 1 { - yield Err(ShellError::string( - "saving to yaml requires a single object (or use --raw)", - )); - } - serde_yaml::to_string(&value_to_yaml_value(&input[0])).unwrap() - } - _ => { - let mut save_data = String::new(); - if input.len() > 0 { - let mut first = true; - for i in input.iter() { - if !first { - save_data.push_str("\n"); - } else { - first = false; - } - save_data.push_str(&i.as_string().unwrap()); - } - } - save_data - } + let content = if !save_raw { + to_string_for(full_path.extension(), &input) + } else { + string_from(&input) }; - let _ = std::fs::write(full_path, contents); + match content { + Ok(save_data) => match std::fs::write(full_path, save_data) { + Ok(o) => o, + Err(e) => yield Err(ShellError::string(e.to_string())), + }, + Err(e) => yield Err(ShellError::string(e.to_string())), + } + }; Ok(OutputStream::new(stream)) } } + +fn string_from(input: &Vec>) -> Result { + let mut save_data = String::new(); + + if input.len() > 0 { + let mut first = true; + for i in input.iter() { + if !first { + save_data.push_str("\n"); + } else { + first = false; + } + if let Ok(data) = &i.as_string() { + save_data.push_str(data); + } + } + } + + Ok(save_data) +} + +fn to_string_for( + ext: Option<&std::ffi::OsStr>, + input: &Vec>, +) -> Result { + let contents = match ext { + Some(x) if x == "csv" => { + if input.len() != 1 { + return Err(ShellError::string( + "saving to csv requires a single object (or use --raw)", + )); + } + to_csv_to_string(&value_to_csv_value(&input[0]))? + } + Some(x) if x == "toml" => { + if input.len() != 1 { + return Err(ShellError::string( + "saving to toml requires a single object (or use --raw)", + )); + } + toml::to_string(&value_to_toml_value(&input[0]))? + } + Some(x) if x == "json" => { + if input.len() != 1 { + return Err(ShellError::string( + "saving to json requires a single object (or use --raw)", + )); + } + serde_json::to_string(&value_to_json_value(&input[0]))? + } + Some(x) if x == "yml" => { + if input.len() != 1 { + return Err(ShellError::string( + "saving to yml requires a single object (or use --raw)", + )); + } + serde_yaml::to_string(&value_to_yaml_value(&input[0]))? + } + Some(x) if x == "yaml" => { + if input.len() != 1 { + return Err(ShellError::string( + "saving to yaml requires a single object (or use --raw)", + )); + } + serde_yaml::to_string(&value_to_yaml_value(&input[0]))? + } + _ => { + return Err(ShellError::string( + "tried saving a single object with an unrecognized format.", + )) + } + }; + + Ok(contents) +} diff --git a/src/errors.rs b/src/errors.rs index e4a263d99..359b472ab 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -411,6 +411,16 @@ impl std::fmt::Display for ShellError { impl std::error::Error for ShellError {} +impl std::convert::From> for ShellError { + fn from(input: Box) -> ShellError { + ProximateShellError::String(StringError { + title: format!("{}", input), + error: Value::nothing(), + }) + .start() + } +} + impl std::convert::From for ShellError { fn from(input: std::io::Error) -> ShellError { ProximateShellError::String(StringError { @@ -431,6 +441,17 @@ impl std::convert::From for ShellError { } } +impl std::convert::From for ShellError { + fn from(input: serde_yaml::Error) -> ShellError { + ProximateShellError::String(StringError { + title: format!("{:?}", input), + error: Value::nothing(), + }) + .start() + } +} + + impl std::convert::From for ShellError { fn from(input: toml::ser::Error) -> ShellError { ProximateShellError::String(StringError {