Review comment: Use combine to parse arguments.

Move over to using Result rather than enums with err

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"

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<String>),
     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<Command, cli::Error> {
+    let arguments = || sep_end_by::<Vec<_>, _, _>(many1(satisfy(|c: char| !c.is_whitespace())), many1::<Vec<_>, _>(space())).expected("arguments");
+
     let help_parser = string(HELP_COMMAND)
-                    .and(skip_many(space()))
-                    .and(many::<Vec<_>, _>(try(any())))
-                    .map(|x| {
-                        let remainder: String = x.1.iter().collect();
-                        let args: Vec<String> = 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::<Vec<_>, _>(try(any())))
-                    .map(|x| {
-                        let remainder: String = x.1.iter().collect();
-                        let args: Vec<String> = 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<Input = _, Output = Command>; 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<Input = _, Output = Result<Command, cli::Error>>; 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<String> = 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<String> = 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));
     }
 }

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)
+        }
+    }
+}

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<InputResult, cli::Error> {
         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))
             }
         }
     }

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
 }

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<String>) -> Repl {
-        Repl{
-            store: Store::new(db_name),
-        }
+    pub fn new(db_name: Option<String>) -> Result<Repl, String> {
+        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!(),
         }
     }

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<String>) -> Store {
+    pub fn new(database: Option<String>) -> Result<Store, cli::Error> {
         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<String>) {
+    pub fn open(&mut self, database: Option<String>) -> 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)
     }
 
 }