From b3ff534690c1ac12b56799aefa848e70ef6ee97c Mon Sep 17 00:00:00 2001 From: Nick Alexander Date: Tue, 16 May 2017 16:40:33 -0700 Subject: [PATCH] Review comment: Use `combine` to parse arguments. Move over to using Result rather than enums with err --- tools/cli/Cargo.toml | 1 + tools/cli/src/mentat_cli/command_parser.rs | 157 ++++++++++----------- tools/cli/src/mentat_cli/errors.rs | 36 +++++ tools/cli/src/mentat_cli/input.rs | 23 +-- tools/cli/src/mentat_cli/lib.rs | 10 +- tools/cli/src/mentat_cli/repl.rs | 59 +++++--- tools/cli/src/mentat_cli/store.rs | 30 ++-- 7 files changed, 189 insertions(+), 127 deletions(-) create mode 100644 tools/cli/src/mentat_cli/errors.rs diff --git a/tools/cli/Cargo.toml b/tools/cli/Cargo.toml index 7523a9b3..58cbc09d 100644 --- a/tools/cli/Cargo.toml +++ b/tools/cli/Cargo.toml @@ -19,6 +19,7 @@ log = "0.3" tempfile = "1.1" combine = "2.2.2" lazy_static = "0.2.2" +error-chain = "0.8.1" [dependencies.rusqlite] version = "0.11" diff --git a/tools/cli/src/mentat_cli/command_parser.rs b/tools/cli/src/mentat_cli/command_parser.rs index fbf7d077..27d8e27f 100644 --- a/tools/cli/src/mentat_cli/command_parser.rs +++ b/tools/cli/src/mentat_cli/command_parser.rs @@ -8,9 +8,25 @@ // CONDITIONS OF ANY KIND, either express or implied. See the License for the // specific language governing permissions and limitations under the License. -use combine::{any, eof, many, many1, sep_by, skip_many, token, Parser}; -use combine::combinator::{choice, try}; -use combine::char::{space, string}; +use combine::{ + eof, + many1, + satisfy, + sep_end_by, + token, + Parser +}; +use combine::char::{ + space, + spaces, + string +}; +use combine::combinator::{ + choice, + try +}; + +use errors as cli; pub static HELP_COMMAND: &'static str = &"help"; pub static OPEN_COMMAND: &'static str = &"open"; @@ -23,58 +39,67 @@ pub enum Command { Help(Vec), Open(String), Close, - Err(String), } impl Command { + /// is_complete returns true if no more input is required for the command to be successfully executed. + /// false is returned if the command is not considered valid. + /// Defaults to true for all commands except Query and Transact. + /// TODO: for query and transact commands, they will be considered complete if a parsable EDN has been entered as an argument pub fn is_complete(&self) -> bool { match self { &Command::Query(_) | &Command::Transact(_) => false, &Command::Help(_) | &Command::Open(_) | - &Command::Close | - &Command::Err(_) => true + &Command::Close => true } } } -pub fn command(s: &str) -> Command { +pub fn command(s: &str) -> Result { + let arguments = || sep_end_by::, _, _>(many1(satisfy(|c: char| !c.is_whitespace())), many1::, _>(space())).expected("arguments"); + let help_parser = string(HELP_COMMAND) - .and(skip_many(space())) - .and(many::, _>(try(any()))) - .map(|x| { - let remainder: String = x.1.iter().collect(); - let args: Vec = remainder.split(" ").filter_map(|s| if s.is_empty() { None } else { Some(s.to_string())}).collect(); - Command::Help(args) + .with(spaces()) + .with(arguments()) + .map(|args| { + Ok(Command::Help(args.clone())) }); let open_parser = string(OPEN_COMMAND) - .and(skip_many(space())) - .and(many1::, _>(try(any()))) - .map(|x| { - let remainder: String = x.1.iter().collect(); - let args: Vec = remainder.split(" ").filter_map(|s| if s.is_empty() { None } else { Some(s.to_string())}).collect(); - if args.len() > 1 { - return Command::Err(format!("Unrecognized argument {:?}", (&args[1]).clone())); + .with(spaces()) + .with(arguments()) + .map(|args| { + if args.len() < 1 { + return Err(cli::ErrorKind::CommandParse("Missing required argument".to_string()).into()); } - Command::Open((&args[0]).clone()) + if args.len() > 1 { + return Err(cli::ErrorKind::CommandParse(format!("Unrecognized argument {:?}", args[1])).into()); + } + Ok(Command::Open(args[0].clone())) }); let close_parser = string(CLOSE_COMMAND) - .and(skip_many(space())) - .map( |_| Command::Close ); + .with(arguments()) + .map(|args| { + if args.len() > 0 { + return Err(cli::ErrorKind::CommandParse(format!("Unrecognized argument {:?}", args[0])).into()); + } + Ok(Command::Close) + }); - skip_many(space()) - .and(token('.')) - .and(choice::<[&mut Parser; 3], _> - ([&mut try(help_parser), - &mut try(open_parser), - &mut try(close_parser),])) - .skip(eof()) - .parse(s) - .map(|x| x.0) - .unwrap_or((((), '0'), Command::Err(format!("Invalid command {:?}", s)))).1 + spaces() + .skip(token('.')) + .with(choice::<[&mut Parser>; 3], _> + ([&mut try(help_parser), + &mut try(open_parser), + &mut try(close_parser),])) + .skip(spaces()) + .skip(eof()) + .parse(s) + .map(|x| x.0) + .unwrap_or(Err(cli::ErrorKind::CommandParse(format!("Invalid command {:?}", s)).into())) } #[cfg(test)] @@ -84,7 +109,7 @@ mod tests { #[test] fn test_help_parser_multiple_args() { let input = ".help command1 command2"; - let cmd = command(&input); + let cmd = command(&input).expect("Expected help command"); match cmd { Command::Help(args) => { assert_eq!(args, vec!["command1", "command2"]); @@ -96,7 +121,7 @@ mod tests { #[test] fn test_help_parser_dot_arg() { let input = ".help .command1"; - let cmd = command(&input); + let cmd = command(&input).expect("Expected help command"); match cmd { Command::Help(args) => { assert_eq!(args, vec![".command1"]); @@ -108,7 +133,7 @@ mod tests { #[test] fn test_help_parser_no_args() { let input = ".help"; - let cmd = command(&input); + let cmd = command(&input).expect("Expected help command"); match cmd { Command::Help(args) => { let empty: Vec = vec![]; @@ -121,7 +146,7 @@ mod tests { #[test] fn test_help_parser_no_args_trailing_whitespace() { let input = ".help "; - let cmd = command(&input); + let cmd = command(&input).expect("Expected help command"); match cmd { Command::Help(args) => { let empty: Vec = vec![]; @@ -134,19 +159,14 @@ mod tests { #[test] fn test_open_parser_multiple_args() { let input = ".open database1 database2"; - let cmd = command(&input); - match cmd { - Command::Err(message) => { - assert_eq!(message, "Unrecognized argument \"database2\""); - }, - _ => assert!(false) - } + let err = command(&input).expect_err("Expected an error"); + assert_eq!(err.to_string(), "Unrecognized argument \"database2\""); } #[test] fn test_open_parser_single_arg() { let input = ".open database1"; - let cmd = command(&input); + let cmd = command(&input).expect("Expected open command"); match cmd { Command::Open(arg) => { assert_eq!(arg, "database1".to_string()); @@ -158,7 +178,7 @@ mod tests { #[test] fn test_open_parser_path_arg() { let input = ".open /path/to/my.db"; - let cmd = command(&input); + let cmd = command(&input).expect("Expected open command"); match cmd { Command::Open(arg) => { assert_eq!(arg, "/path/to/my.db".to_string()); @@ -170,7 +190,7 @@ mod tests { #[test] fn test_open_parser_file_arg() { let input = ".open my.db"; - let cmd = command(&input); + let cmd = command(&input).expect("Expected open command"); match cmd { Command::Open(arg) => { assert_eq!(arg, "my.db".to_string()); @@ -182,31 +202,21 @@ mod tests { #[test] fn test_open_parser_no_args() { let input = ".open"; - let cmd = command(&input); - match cmd { - Command::Err(message) => { - assert_eq!(message, format!("Invalid command {:?}", input)); - }, - _ => assert!(false) - } + let err = command(&input).expect_err("Expected an error"); + assert_eq!(err.to_string(), "Missing required argument"); } #[test] fn test_close_parser_with_args() { let input = ".close arg1"; - let cmd = command(&input); - match cmd { - Command::Err(message) => { - assert_eq!(message, format!("Invalid command {:?}", input)); - }, - _ => assert!(false) - } + let err = command(&input).expect_err("Expected an error"); + assert_eq!(err.to_string(), format!("Invalid command {:?}", input)); } #[test] fn test_close_parser_no_args() { let input = ".close"; - let cmd = command(&input); + let cmd = command(&input).expect("Expected close command"); match cmd { Command::Close => assert!(true), _ => assert!(false) @@ -216,7 +226,7 @@ mod tests { #[test] fn test_close_parser_no_args_trailing_whitespace() { let input = ".close "; - let cmd = command(&input); + let cmd = command(&input).expect("Expected close command"); match cmd { Command::Close => assert!(true), _ => assert!(false) @@ -226,7 +236,7 @@ mod tests { #[test] fn test_parser_preceeding_trailing_whitespace() { let input = " .close "; - let cmd = command(&input); + let cmd = command(&input).expect("Expected close command"); match cmd { Command::Close => assert!(true), _ => assert!(false) @@ -236,25 +246,14 @@ mod tests { #[test] fn test_command_parser_no_dot() { let input = "help command1 command2"; - let cmd = command(&input); - match cmd { - Command::Err(message) => { - assert_eq!(message, format!("Invalid command {:?}", input)); - }, - _ => assert!(false) - } + let err = command(&input).expect_err("Expected an error"); + assert_eq!(err.to_string(), format!("Invalid command {:?}", input)); } #[test] fn test_command_parser_invalid_cmd() { let input = ".foo command1"; - let cmd = command(&input); - match cmd { - Command::Err(message) => { - assert_eq!(message, format!("Invalid command {:?}", input)); - }, - _ => assert!(false) - } - + let err = command(&input).expect_err("Expected an error"); + assert_eq!(err.to_string(), format!("Invalid command {:?}", input)); } } diff --git a/tools/cli/src/mentat_cli/errors.rs b/tools/cli/src/mentat_cli/errors.rs new file mode 100644 index 00000000..8e095ab3 --- /dev/null +++ b/tools/cli/src/mentat_cli/errors.rs @@ -0,0 +1,36 @@ +// Copyright 2016 Mozilla +// +// Licensed under the Apache License, Version 2.0 (the "License"); you may not use +// this file except in compliance with the License. You may obtain a copy of the +// License at http://www.apache.org/licenses/LICENSE-2.0 +// Unless required by applicable law or agreed to in writing, software distributed +// under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR +// CONDITIONS OF ANY KIND, either express or implied. See the License for the +// specific language governing permissions and limitations under the License. + +#![allow(dead_code)] + +use rusqlite; + +use mentat::errors as mentat; + +error_chain! { + types { + Error, ErrorKind, ResultExt, Result; + } + + foreign_links { + Rusqlite(rusqlite::Error); + } + + links { + MentatError(mentat::Error, mentat::ErrorKind); + } + + errors { + CommandParse(message: String) { + description("An error occured parsing the entered command") + display("{}", message) + } + } +} diff --git a/tools/cli/src/mentat_cli/input.rs b/tools/cli/src/mentat_cli/input.rs index a03f2939..0679b3b6 100644 --- a/tools/cli/src/mentat_cli/input.rs +++ b/tools/cli/src/mentat_cli/input.rs @@ -8,14 +8,19 @@ // CONDITIONS OF ANY KIND, either express or implied. See the License for the // specific language governing permissions and limitations under the License. -use std::io::{stdin}; +use std::io::stdin; use linefeed::Reader; use linefeed::terminal::DefaultTerminal; use self::InputResult::*; -use command_parser::{Command, command}; +use command_parser::{ + Command, + command +}; + +use errors as cli; /// Possible results from reading input from `InputReader` #[derive(Clone, Debug)] @@ -28,8 +33,6 @@ pub enum InputResult { More(Command), /// End of file reached Eof, - /// Error while parsing input; - InputError(String), } /// Reads input from `stdin` @@ -63,30 +66,30 @@ impl InputReader { /// Reads a single command, item, or statement from `stdin`. /// Returns `More` if further input is required for a complete result. /// In this case, the input received so far is buffered internally. - pub fn read_input(&mut self, prompt: &str) -> InputResult { + pub fn read_input(&mut self, prompt: &str) -> Result { let line = match self.read_line(prompt) { Some(s) => s, - None => return Eof, + None => return Ok(Eof), }; self.buffer.push_str(&line); if self.buffer.is_empty() { - return Empty; + return Ok(Empty); } self.add_history(&line); - let cmd = command(&self.buffer); + let cmd = try!(command(&self.buffer)); match cmd { Command::Query(_) | Command::Transact(_) if !cmd.is_complete() => { - More(cmd) + Ok(More(cmd)) }, _ => { self.buffer.clear(); - InputResult::MetaCommand(cmd) + Ok(InputResult::MetaCommand(cmd)) } } } diff --git a/tools/cli/src/mentat_cli/lib.rs b/tools/cli/src/mentat_cli/lib.rs index 49533170..76f7d602 100644 --- a/tools/cli/src/mentat_cli/lib.rs +++ b/tools/cli/src/mentat_cli/lib.rs @@ -12,6 +12,7 @@ #[macro_use] extern crate log; #[macro_use] extern crate lazy_static; +#[macro_use] extern crate error_chain; extern crate combine; extern crate env_logger; @@ -27,6 +28,7 @@ pub mod command_parser; pub mod store; pub mod input; pub mod repl; +pub mod errors; pub fn run() -> i32 { env_logger::init().unwrap(); @@ -58,8 +60,12 @@ pub fn run() -> i32 { let db_name = matches.opt_str("d"); - let mut repl = repl::Repl::new(db_name); - repl.run(); + let repl = repl::Repl::new(db_name); + if repl.is_ok() { + repl.unwrap().run(); + } else { + println!("{}", repl.err().unwrap()); + } 0 } diff --git a/tools/cli/src/mentat_cli/repl.rs b/tools/cli/src/mentat_cli/repl.rs index fdc82503..0ad0b99e 100644 --- a/tools/cli/src/mentat_cli/repl.rs +++ b/tools/cli/src/mentat_cli/repl.rs @@ -10,10 +10,22 @@ use std::collections::HashMap; -use command_parser::{Command, HELP_COMMAND, OPEN_COMMAND}; -use input::{InputReader}; -use input::InputResult::{MetaCommand, Empty, More, Eof, InputError}; -use store::Store; +use command_parser::{ + Command, + HELP_COMMAND, + OPEN_COMMAND +}; +use input::InputReader; +use input::InputResult::{ + MetaCommand, + Empty, + More, + Eof +}; +use store::{ + Store, + db_output_name +}; /// Starting prompt const DEFAULT_PROMPT: &'static str = "mentat=> "; @@ -37,10 +49,11 @@ pub struct Repl { impl Repl { /// Constructs a new `Repl`. - pub fn new(db_name: Option) -> Repl { - Repl{ - store: Store::new(db_name), - } + pub fn new(db_name: Option) -> Result { + let store = try!(Store::new(db_name.clone()).map_err(|e| e.to_string())); + Ok(Repl{ + store: store, + }) } /// Runs the REPL interactively. @@ -50,26 +63,22 @@ impl Repl { loop { let res = input.read_input(if more.is_some() { MORE_PROMPT } else { DEFAULT_PROMPT }); - match res { - MetaCommand(cmd) => { + Ok(MetaCommand(cmd)) => { debug!("read command: {:?}", cmd); more = None; self.handle_command(cmd); }, - Empty => (), - More(cmd) => { more = Some(cmd); }, - Eof => { + Ok(Empty) => (), + Ok(More(cmd)) => { more = Some(cmd); }, + Ok(Eof) => { if input.is_tty() { println!(""); } break; }, - InputError(err) => { - println!("{}", err); - more = None; - }, - }; + Err(e) => println!("{}", e.to_string()), + } } } @@ -78,10 +87,18 @@ impl Repl { match cmd { Command::Help(args) => self.help_command(args), Command::Open(db) => { - self.store.open(Some(db)); + match self.store.open(Some(db.clone())) { + Ok(_) => println!("Database {:?} opened", db_output_name(&db)), + Err(e) => println!("{}", e.to_string()) + }; + }, + Command::Close => { + let old_db_name = self.store.db_name.clone(); + match self.store.close() { + Ok(_) => println!("Database {:?} closed", db_output_name(&old_db_name)), + Err(e) => println!("{}", e.to_string()) + }; }, - Command::Close => self.store.close(), - Command::Err(message) => println!("{}", message), _ => unimplemented!(), } } diff --git a/tools/cli/src/mentat_cli/store.rs b/tools/cli/src/mentat_cli/store.rs index 7243e4db..a1689f63 100644 --- a/tools/cli/src/mentat_cli/store.rs +++ b/tools/cli/src/mentat_cli/store.rs @@ -15,37 +15,37 @@ use mentat::{ use mentat::conn::Conn; +use errors as cli; + pub struct Store { handle: rusqlite::Connection, conn: Conn, - db_name: String, + pub db_name: String, } -fn db_output_name(db_name: &String) -> String { +pub fn db_output_name(db_name: &String) -> String { if db_name.is_empty() { "in-memory db".to_string() } else { db_name.clone() } } impl Store { - pub fn new(database: Option) -> Store { + pub fn new(database: Option) -> Result { let db_name = database.unwrap_or("".to_string()); - let mut handle = new_connection(&db_name).expect("Couldn't open conn."); - let conn = Conn::connect(&mut handle).expect("Couldn't open DB."); - println!("Database {:?} opened", db_output_name(&db_name)); - Store { handle, conn, db_name } + + let mut handle = try!(new_connection(&db_name)); + let conn = try!(Conn::connect(&mut handle)); + Ok(Store { handle, conn, db_name }) } - pub fn open(&mut self, database: Option) { + pub fn open(&mut self, database: Option) -> Result<(), cli::Error> { self.db_name = database.unwrap_or("".to_string()); - self.handle = new_connection(&self.db_name).expect("Couldn't open conn."); - self.conn = Conn::connect(&mut self.handle).expect("Couldn't open DB."); - println!("Database {:?} opened", db_output_name(&self.db_name)); + self.handle = try!(new_connection(&self.db_name)); + self.conn = try!(Conn::connect(&mut self.handle)); + Ok(()) } - pub fn close(&mut self) { - self.handle = new_connection("").expect("Couldn't close conn."); - self.conn = Conn::connect(&mut self.handle).expect("Couldn't close DB."); - println!("Database {:?} closed", db_output_name(&self.db_name)); + pub fn close(&mut self) -> Result<(), cli::Error> { self.db_name = "".to_string(); + self.open(None) } }