Fix database error name. Improve dice number conversion error message.

This follows the example in the Combine documentation to use impl
trait return types so we can isolate the parsers into their own
functions.

Cargo.lock

@@ -840,7 +840,7 @@ dependencies = [
  "httparse",
  "httpdate",
  "itoa",
- "pin-project 1.0.4",
+ "pin-project 1.0.5",
  "socket2",
  "tokio",
  "tower-service",
@@ -1036,7 +1036,7 @@ checksum = "7ffc5c5338469d4d3ea17d269fa8ea3512ad247247c30bd2df69e68309ed0a08"
 [[package]]
 name = "matrix-sdk"
 version = "0.2.0"
-source = "git+https://github.com/matrix-org/matrix-rust-sdk?branch=master#b66c666997cbcf32071d24ffe2484a215de8d0e4"
+source = "git+https://github.com/matrix-org/matrix-rust-sdk?branch=master#bdaed6237e104b738b5ad25c063675373d9fa60d"
 dependencies = [
  "dashmap",
  "futures",
@@ -1058,7 +1058,7 @@ dependencies = [
 [[package]]
 name = "matrix-sdk-base"
 version = "0.2.0"
-source = "git+https://github.com/matrix-org/matrix-rust-sdk?branch=master#b66c666997cbcf32071d24ffe2484a215de8d0e4"
+source = "git+https://github.com/matrix-org/matrix-rust-sdk?branch=master#bdaed6237e104b738b5ad25c063675373d9fa60d"
 dependencies = [
  "chacha20poly1305",
  "dashmap",
@@ -1081,7 +1081,7 @@ dependencies = [
 [[package]]
 name = "matrix-sdk-common"
 version = "0.2.0"
-source = "git+https://github.com/matrix-org/matrix-rust-sdk?branch=master#b66c666997cbcf32071d24ffe2484a215de8d0e4"
+source = "git+https://github.com/matrix-org/matrix-rust-sdk?branch=master#bdaed6237e104b738b5ad25c063675373d9fa60d"
 dependencies = [
  "async-trait",
  "futures-locks",
@@ -1095,7 +1095,7 @@ dependencies = [
 [[package]]
 name = "matrix-sdk-crypto"
 version = "0.2.0"
-source = "git+https://github.com/matrix-org/matrix-rust-sdk?branch=master#b66c666997cbcf32071d24ffe2484a215de8d0e4"
+source = "git+https://github.com/matrix-org/matrix-rust-sdk?branch=master#bdaed6237e104b738b5ad25c063675373d9fa60d"
 dependencies = [
  "aes-ctr",
  "aes-gcm",
@@ -1425,11 +1425,11 @@ dependencies = [
 
 [[package]]
 name = "pin-project"
-version = "1.0.4"
+version = "1.0.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "95b70b68509f17aa2857863b6fa00bf21fc93674c7a8893de2f469f6aa7ca2f2"
+checksum = "96fa8ebb90271c4477f144354485b8068bd8f6b78b428b01ba892ca26caf0b63"
 dependencies = [
- "pin-project-internal 1.0.4",
+ "pin-project-internal 1.0.5",
 ]
 
 [[package]]
@@ -1445,9 +1445,9 @@ dependencies = [
 
 [[package]]
 name = "pin-project-internal"
-version = "1.0.4"
+version = "1.0.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "caa25a6393f22ce819b0f50e0be89287292fda8d425be38ee0ca14c4931d9e71"
+checksum = "758669ae3558c6f74bd2a18b41f7ac0b5a195aea6639d6a9b5e5d1ad5ba24c0b"
 dependencies = [
  "proc-macro2 1.0.24",
  "quote 1.0.8",

Cargo.toml

@@ -22,7 +22,7 @@ async-trait = "0.1"
 url = "2.1"
 dirs = "3.0"
 indoc = "1.0"
-combine = "4.3"
+combine = "4.5"
 sled = "0.34"
 zerocopy = "0.3"
 byteorder = "1.3"

src/error.rs

@@ -68,7 +68,7 @@ pub enum BotError {
     VariablesNotSupported,
 
     #[error("database error")]
-    DatabaseErrror(#[from] sled::Error),
+    DatabaseError(#[from] sled::Error),
 
     #[error("too many commands or message was too large")]
     MessageTooLarge,

src/parser.rs

@@ -1,11 +1,11 @@
+use combine::error::ParseError;
 use combine::parser::char::{digit, letter, spaces};
+use combine::stream::Stream;
 use combine::{many, many1, one_of, Parser};
 use thiserror::Error;
 
-//******************************
+/// Errors for dice parsing.
-//New hotness
+#[derive(Debug, Clone, PartialEq, Copy, Error)]
-//******************************
-#[derive(Debug, Clone, Copy, PartialEq, Error)]
 pub enum DiceParsingError {
     #[error("invalid amount")]
     InvalidAmount,
@@ -18,8 +18,21 @@ pub enum DiceParsingError {
 
     #[error("{0}")]
     InternalParseError(#[from] combine::error::StringStreamError),
+
+    #[error("number parsing error (too large?)")]
+    ConversionError,
 }
 
+impl From<std::num::ParseIntError> for DiceParsingError {
+    fn from(_error: std::num::ParseIntError) -> Self {
+        DiceParsingError::ConversionError
+    }
+}
+
+type ParseResult<T> = Result<T, DiceParsingError>;
+
+/// A parsed operator for a number. Whether to add or remove it from
+/// the total amount of dice rolled.
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum Operator {
     Plus,
@@ -27,6 +40,8 @@ pub enum Operator {
 }
 
 impl Operator {
+    /// Calculate multiplier for how to convert the number. Returns 1
+    /// for positive, and -1 for negative.
     pub fn mult(&self) -> i32 {
         match self {
             Operator::Plus => 1,
@@ -35,18 +50,98 @@ impl Operator {
     }
 }
 
+/// One part of the dice amount in an expression. Can be a number or a
+/// variable name.
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum Element {
+    /// This element in the expression is a variable, which will be
+    /// resolved to a number by consulting the dtaabase.
     Variable(String),
+
+    /// This element is a simple number, and will be added or
+    /// subtracted from the total dice amount depending on its
+    /// corresponding Operator.
     Number(i32),
 }
 
+/// One part of the parsed dice rolling expression. Combines an
+/// operator and an element into one struct. Examples of Amounts would
+/// be "+4" or "- myvariable", which translate to Operator::Plus and
+/// Element::Number(4), and Operator::Minus and
+/// Element::Variable("myvariable"), respectively.
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub struct Amount {
     pub operator: Operator,
     pub element: Element,
 }
 
+/// Parser that attempt to convert the text at the start of the dice
+/// parsing into an Amount instance.
+fn first_amount_parser<Input>() -> impl Parser<Input, Output = ParseResult<Amount>>
+where
+    Input: Stream<Token = char>,
+    Input::Error: ParseError<Input::Token, Input::Range, Input::Position>,
+{
+    let map_first_amount = |value: String| {
+        if value.chars().all(char::is_numeric) {
+            let num = value.parse::<i32>()?;
+            Ok(Amount {
+                operator: Operator::Plus,
+                element: Element::Number(num),
+            })
+        } else {
+            Ok(Amount {
+                operator: Operator::Plus,
+                element: Element::Variable(value),
+            })
+        }
+    };
+
+    many1(letter())
+        .or(many1(digit()))
+        .skip(spaces().silent()) //Consume any space after first amount
+        .map(map_first_amount)
+}
+
+/// Attempt to convert some text in the middle or end of the dice roll
+/// string into an Amount.
+fn amount_parser<Input>() -> impl Parser<Input, Output = ParseResult<Amount>>
+where
+    Input: Stream<Token = char>,
+    Input::Error: ParseError<Input::Token, Input::Range, Input::Position>,
+{
+    let plus_or_minus = one_of("+-".chars());
+    let parse_operator = plus_or_minus.map(|sign: char| match sign {
+        '+' => Operator::Plus,
+        '-' => Operator::Minus,
+        _ => Operator::Plus,
+    });
+
+    // Element must either be a proper i32, or a variable name.
+    let map_element = |value: String| -> ParseResult<Element> {
+        if value.chars().all(char::is_numeric) {
+            let num = value.parse::<i32>()?;
+            Ok(Element::Number(num))
+        } else {
+            Ok(Element::Variable(value))
+        }
+    };
+
+    let parse_element = many1(letter()).or(many1(digit())).map(map_element);
+
+    let element_parser = parse_operator
+        .skip(spaces().silent())
+        .and(parse_element)
+        .skip(spaces().silent());
+
+    let convert_to_amount = |(operator, element_result)| match element_result {
+        Ok(element) => Ok(Amount { operator, element }),
+        Err(e) => Err(e),
+    };
+
+    element_parser.map(convert_to_amount)
+}
+
 /// Parse an expression of numbers and/or variables into elements
 /// coupled with operators, where an operator is "+" or "-", and an
 /// element is either a number or variable name. The first element
@@ -57,55 +152,15 @@ pub struct Amount {
 /// utilzing this function should layer their own checks on top of
 /// this; perhaps they do not want more than one expression, or some
 /// other rules.
-pub fn parse_amounts(input: &str) -> Result<Vec<Amount>, DiceParsingError> {
+pub fn parse_amounts(input: &str) -> ParseResult<Vec<Amount>> {
     let input = input.trim();
 
-    let plus_or_minus = one_of("+-".chars());
+    let remaining_amounts = many(amount_parser()).map(|amounts: Vec<ParseResult<Amount>>| amounts);
-    let maybe_sign = plus_or_minus.map(|sign: char| match sign {
+    let mut parser = first_amount_parser().and(remaining_amounts);
-        '+' => Operator::Plus,
-        '-' => Operator::Minus,
-        _ => Operator::Plus,
-    });
 
-    //TODO make this a macro or something
+    // Collapses first amount + remaining amounts into a single Vec,
-    let first = many1(letter())
+    // while collecting extraneous input.
-        .or(many1(digit()))
+    type ParsedAmountExpr = (ParseResult<Amount>, Vec<ParseResult<Amount>>);
-        .skip(spaces().silent()) //Consume any space after first amount
-        .map(|value: String| match value.parse::<i32>() {
-            Ok(num) => Amount {
-                operator: Operator::Plus,
-                element: Element::Number(num),
-            },
-            _ => Amount {
-                operator: Operator::Plus,
-                element: Element::Variable(value),
-            },
-        });
-
-    let variable_or_number =
-        many1(letter())
-            .or(many1(digit()))
-            .map(|value: String| match value.parse::<i32>() {
-                Ok(num) => Element::Number(num),
-                _ => Element::Variable(value),
-            });
-
-    let sign_and_word = maybe_sign
-        .skip(spaces().silent())
-        .and(variable_or_number)
-        .skip(spaces().silent())
-        .map(|parsed: (Operator, Element)| Amount {
-            operator: parsed.0,
-            element: parsed.1,
-        });
-
-    let rest = many(sign_and_word).map(|expr: Vec<_>| expr);
-
-    let mut parser = first.and(rest);
-
-    //Maps the found expression into a Vec of Amount instances,
-    //tacking the first one on.
-    type ParsedAmountExpr = (Amount, Vec<Amount>);
     let (results, rest) = parser
         .parse(input)
         .map(|mut results: (ParsedAmountExpr, &str)| {
@@ -115,7 +170,8 @@ pub fn parse_amounts(input: &str) -> Result<Vec<Amount>, DiceParsingError> {
         })?;
 
     if rest.len() == 0 {
-        Ok(results)
+        // Any ParseResult errors will short-circuit the collect.
+        results.into_iter().collect()
     } else {
         Err(DiceParsingError::UnconsumedInput)
     }
@@ -149,6 +205,15 @@ mod tests {
         );
     }
 
+    #[test]
+    fn parsing_huge_number_should_error() {
+        // A number outside the bounds of i32 should not be a valid
+        // parse.
+        let result = parse_amounts("159875294375198734982379875392");
+        assert!(result.is_err());
+        assert!(result.unwrap_err() == DiceParsingError::ConversionError);
+    }
+
     #[test]
     fn parse_single_variable_amount_test() {
         let result = parse_amounts("asdf");