Dice pool and command parser rewrite to prepare for user variables.

This commit refactors the parsing and rolling for the dice pool system
to prepare for support of user variables. The nom parser was dropped
in favor of the easier-to-understand combine parser in most parts of
the code.

A breaking change was introduced into the dice pool syntax to allow
for proper expressions and variables. The syntax is now
"modifiers:pool-amount", e.g. "n:gnosis+8". The simple single-number
syntax with no modifiers is also still understood.

Dice pool expressions are translated into a Vec of "Amount" objects,
stored by the DicePool struct. They have an operator (+ or -) and
either a number or variable name. When the dice pool is rolled, this
list of Amonuts are is collapsed into a single number that is rolled,
as it was before the refactor.

The following changes were made to the dice rolling code:
 - Store Vec<Amount> on DicePool instead of single number to roll.
 - New struct RolledDicePool to store result of a dice pool roll.
 - Remove Display trait from DicePool, move it over to RolledDicePool.
 - Separate extra dice pool info into DicePoolModifiers.
 - DicePoolModifiers is shared between DicePool and RolledDicePool.
 - Dice parsing and rolling now return standard Result objects.

This commit does NOT enable support of actually using variables. Any
dice pool roll containing a variable will result in an eror.

The command parser was also rewritten to use combine and rely on the
standard Result pattern.

.gitignore

@@ -1,6 +1,7 @@
 target/
 dicebot-config
 todo
+todo.org
 cache
 *.tar
 *.tar.gz

Cargo.lock

@@ -277,6 +277,7 @@ dependencies = [
  "actix",
  "actix-rt",
  "async-trait",
+ "combine",
  "dirs",
  "env_logger",
  "indoc",
@@ -287,6 +288,7 @@ dependencies = [
  "matrix-sdk-common 0.1.0 (git+https://github.com/matrix-org/matrix-rust-sdk?rev=0.1.0)",
  "matrix-sdk-common-macros",
  "nom",
+ "once_cell",
  "rand",
  "serde",
  "thiserror",
@@ -325,6 +327,17 @@ dependencies = [
  "cc",
 ]
 
+[[package]]
+name = "combine"
+version = "4.3.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2809f67365382d65fd2b6d9c22577231b954ed27400efeafbe687bda75abcc0b"
+dependencies = [
+ "bytes",
+ "memchr",
+ "pin-project-lite",
+]
+
 [[package]]
 name = "const-random"
 version = "0.1.8"

Cargo.toml

@@ -24,6 +24,8 @@ dirs = "3.0"
 indoc = "1.0"
 actix = "0.10"
 actix-rt = "1.1"
+combine = "4.3"
+once_cell = "1.4"
 
 # The versioning of the matrix SDK follows its Cargo.toml. The SDK and
 # macros are on master, but it imports the common and base from 0.1.0.

src/cofd/dice.rs

@@ -1,8 +1,36 @@
+use crate::error::BotError;
 use crate::roll::{Roll, Rolled};
 use itertools::Itertools;
 use std::convert::TryFrom;
 use std::fmt;
 
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum Operator {
+    Plus,
+    Minus,
+}
+
+impl Operator {
+    pub fn mult(&self) -> i32 {
+        match self {
+            Operator::Plus => 1,
+            Operator::Minus => -1,
+        }
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum Element {
+    Variable(String),
+    Number(i32),
+}
+
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub struct Amount {
+    pub operator: Operator,
+    pub element: Element,
+}
+
 #[derive(Debug, PartialEq, Eq, Clone, Copy)]
 pub enum DicePoolQuality {
     TenAgain,
@@ -26,58 +54,145 @@ impl fmt::Display for DicePoolQuality {
     }
 }
 
-#[derive(Debug, PartialEq, Eq, Clone)]
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
-pub struct DicePool {
+pub struct DicePoolModifiers {
-    pub(crate) count: u32,
+    pub(crate) success_on: i32,
-    pub(crate) sides: u32,
+    pub(crate) exceptional_on: i32,
-    pub(crate) success_on: u32,
-    pub(crate) exceptional_success: u32,
     pub(crate) quality: DicePoolQuality,
 }
 
-impl fmt::Display for DicePool {
+impl DicePoolModifiers {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+    pub fn default() -> DicePoolModifiers {
-        write!(
+        DicePoolModifiers {
-            f,
+            success_on: 8,
-            "{} dice ({}, exceptional on {} successes)",
+            exceptional_on: 5,
-            self.count, self.quality, self.exceptional_success
+            quality: DicePoolQuality::TenAgain,
-        )
         }
     }
 
-impl DicePool {
+    pub fn custom_quality(quality: DicePoolQuality) -> DicePoolModifiers {
-    pub fn new(count: u32, successes_for_exceptional: u32, quality: DicePoolQuality) -> DicePool {
+        let success_on = if quality != DicePoolQuality::ChanceDie {
-        DicePool {
+            8
-            count: count,
+        } else {
-            sides: 10, //TODO make configurable
+            10
-            //TODO make configurable
+        };
-            success_on: match quality {
+        DicePoolModifiers {
-                DicePoolQuality::ChanceDie => 10,
+            success_on: success_on,
-                _ => 8,
+            exceptional_on: 5,
-            },
-            exceptional_success: successes_for_exceptional,
             quality: quality,
         }
     }
 
-    pub fn chance_die() -> DicePool {
+    pub fn custom_exceptional_on(exceptional_on: i32) -> DicePoolModifiers {
-        DicePool {
+        DicePoolModifiers {
-            count: 1,
+            success_on: 8,
-            sides: 10,
+            exceptional_on: exceptional_on,
-            success_on: 10,
+            quality: DicePoolQuality::TenAgain,
-            exceptional_success: 5,
-            quality: DicePoolQuality::ChanceDie,
         }
     }
+
+    pub fn custom(quality: DicePoolQuality, exceptional_on: i32) -> DicePoolModifiers {
+        DicePoolModifiers {
+            success_on: 8,
+            exceptional_on: exceptional_on,
+            quality: quality,
+        }
+    }
+}
+
+#[derive(Debug, PartialEq)]
+pub struct DicePool {
+    pub(crate) amounts: Vec<Amount>,
+    pub(crate) sides: i32,
+    pub(crate) modifiers: DicePoolModifiers,
+}
+
+fn calculate_dice_amount(amounts: &Vec<Amount>) -> Result<i32, BotError> {
+    let dice_amount: Result<i32, BotError> = amounts
+        .iter()
+        .map(|amount| match &amount.element {
+            Element::Number(num_dice) => Ok(*num_dice * amount.operator.mult()),
+            Element::Variable(variable) => handle_variable(&variable),
+        })
+        .collect::<Result<Vec<i32>, _>>()
+        .map(|numbers| numbers.iter().sum());
+
+    dice_amount
+}
+
+impl DicePool {
+    pub fn easy_pool(dice_amount: i32, quality: DicePoolQuality) -> DicePool {
+        DicePool {
+            amounts: vec![Amount {
+                operator: Operator::Plus,
+                element: Element::Number(dice_amount),
+            }],
+            sides: 10,
+            modifiers: DicePoolModifiers::custom_quality(quality),
+        }
+    }
+
+    pub fn easy_with_modifiers(dice_amount: i32, modifiers: DicePoolModifiers) -> DicePool {
+        DicePool {
+            amounts: vec![Amount {
+                operator: Operator::Plus,
+                element: Element::Number(dice_amount),
+            }],
+            sides: 10,
+            modifiers: modifiers,
+        }
+    }
+
+    pub fn new(amounts: Vec<Amount>, modifiers: DicePoolModifiers) -> DicePool {
+        DicePool {
+            amounts: amounts,
+            sides: 10, //TODO make configurable
+            //TODO make configurable
+            modifiers: modifiers,
+        }
+    }
+
+    pub fn chance_die() -> DicePool {
+        DicePool::easy_pool(1, DicePoolQuality::ChanceDie)
+    }
+}
+
+///The result of a successfully executed roll of a dice pool. Does not
+///contain the heavy information of the DicePool instance.
+pub struct RolledDicePool {
+    pub(crate) num_dice: i32,
+    pub(crate) roll: DicePoolRoll,
+    pub(crate) modifiers: DicePoolModifiers,
+}
+
+impl RolledDicePool {
+    fn from(pool: &DicePool, num_dice: i32, rolls: Vec<i32>) -> RolledDicePool {
+        RolledDicePool {
+            modifiers: pool.modifiers,
+            num_dice: num_dice,
+            roll: DicePoolRoll {
+                rolls: rolls,
+                modifiers: pool.modifiers,
+            },
+        }
+    }
+}
+
+impl fmt::Display for RolledDicePool {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "{} dice ({}, exceptional on {} successes)",
+            self.num_dice, self.modifiers.quality, self.modifiers.exceptional_on
+        )
+    }
 }
 
 ///Store all rolls of the dice pool dice into one struct.
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub struct DicePoolRoll {
-    quality: DicePoolQuality,
+    modifiers: DicePoolModifiers,
-    success_on: u32,
+    rolls: Vec<i32>,
-    exceptional_on: u32,
-    rolls: Vec<u32>,
 }
 
 fn fmt_rolls(pool: &DicePoolRoll) -> String {
@@ -96,7 +211,7 @@ fn fmt_rolls(pool: &DicePoolRoll) -> String {
 }
 
 fn fmt_for_failure(pool: &DicePoolRoll) -> String {
-    match pool.quality {
+    match pool.modifiers.quality {
         //There should only be 1 die in a chance die roll.
         DicePoolQuality::ChanceDie if pool.rolls().first() == Some(&1) => {
             String::from("dramatic failure!")
@@ -106,7 +221,7 @@ fn fmt_for_failure(pool: &DicePoolRoll) -> String {
 }
 
 impl DicePoolRoll {
-    pub fn rolls(&self) -> &[u32] {
+    pub fn rolls(&self) -> &[i32] {
         &self.rolls
     }
 
@@ -115,20 +230,20 @@ impl DicePoolRoll {
             .rolls
             .iter()
             .cloned()
-            .filter(|&roll| roll >= self.success_on)
+            .filter(|&roll| roll >= self.modifiers.success_on)
             .count();
         i32::try_from(successes).unwrap_or(0)
     }
 
     pub fn is_exceptional(&self) -> bool {
-        self.successes() >= (self.exceptional_on as i32)
+        self.successes() >= self.modifiers.exceptional_on
     }
 }
 
 impl Roll for DicePool {
-    type Output = DicePoolRoll;
+    type Output = Result<RolledDicePool, BotError>;
 
-    fn roll(&self) -> DicePoolRoll {
+    fn roll(&self) -> Result<RolledDicePool, BotError> {
         roll_dice(self, &mut RngDieRoller(rand::thread_rng()))
     }
 }
@@ -159,14 +274,14 @@ impl fmt::Display for DicePoolRoll {
 }
 
 trait DieRoller {
-    fn roll_number(&mut self, sides: u32) -> u32;
+    fn roll_number(&mut self, sides: i32) -> i32;
 }
 
 ///A version of DieRoller that uses a rand::Rng to roll numbers.
 struct RngDieRoller<R: rand::Rng>(R);
 
 impl<R: rand::Rng> DieRoller for RngDieRoller<R> {
-    fn roll_number(&mut self, sides: u32) -> u32 {
+    fn roll_number(&mut self, sides: i32) -> i32 {
         self.0.gen_range(1, sides + 1)
     }
 }
@@ -176,9 +291,9 @@ impl<R: rand::Rng> DieRoller for RngDieRoller<R> {
 ///Statistically speaking, usually one result will be returned from this function.
 fn roll_exploding_die<R: DieRoller>(
     roller: &mut R,
-    sides: u32,
+    sides: i32,
-    explode_on_or_higher: u32,
+    explode_on_or_higher: i32,
-) -> Vec<u32> {
+) -> Vec<i32> {
     let mut results = vec![];
     loop {
         let roll = roller.roll_number(sides);
@@ -193,7 +308,7 @@ fn roll_exploding_die<R: DieRoller>(
 ///A die with the rote quality is re-rolled once if the roll fails. Otherwise, it obeys
 ///all normal rules (re-roll 10s). Re-rolled dice are appended to the result set, so we
 ///can keep track of the actual dice that were rolled.
-fn roll_rote_die<R: DieRoller>(roller: &mut R, sides: u32, success_on: u32) -> Vec<u32> {
+fn roll_rote_die<R: DieRoller>(roller: &mut R, sides: i32, success_on: i32) -> Vec<i32> {
     let mut rolls = roll_exploding_die(roller, sides, 10);
 
     if rolls.len() == 1 && rolls[0] < success_on {
@@ -208,15 +323,16 @@ fn roll_rote_die<R: DieRoller>(roller: &mut R, sides: u32, success_on: u32) -> V
 ///there are multiple 10s). Nine- and eight-again will explode similarly if the result is
 ///at least that number. Rote quality will re-roll a failure once, while also exploding
 ///on 10. The function returns a Vec of all rolled dice (usually 1).
-fn roll_die<R: DieRoller>(roller: &mut R, pool: &DicePool) -> Vec<u32> {
+fn roll_die<R: DieRoller>(roller: &mut R, pool: &DicePool) -> Vec<i32> {
     let mut results = vec![];
     let sides = pool.sides;
+    let success_on = pool.modifiers.success_on;
 
-    match pool.quality {
+    match pool.modifiers.quality {
         DicePoolQuality::TenAgain => results.append(&mut roll_exploding_die(roller, sides, 10)),
         DicePoolQuality::NineAgain => results.append(&mut roll_exploding_die(roller, sides, 9)),
         DicePoolQuality::EightAgain => results.append(&mut roll_exploding_die(roller, sides, 8)),
-        DicePoolQuality::Rote => results.append(&mut roll_rote_die(roller, sides, pool.success_on)),
+        DicePoolQuality::Rote => results.append(&mut roll_rote_die(roller, sides, success_on)),
         DicePoolQuality::ChanceDie | DicePoolQuality::NoExplode => {
             results.push(roller.roll_number(sides))
         }
@@ -225,19 +341,16 @@ fn roll_die<R: DieRoller>(roller: &mut R, pool: &DicePool) -> Vec<u32> {
     results
 }
 
+fn handle_variable(_variable: &str) -> Result<i32, BotError> {
+    Err(BotError::VariablesNotSupported)
+}
+
 ///Roll the dice in a dice pool, according to behavior documented in the various rolling
 ///methods.
-fn roll_dice<R: DieRoller>(pool: &DicePool, roller: &mut R) -> DicePoolRoll {
+fn roll_dice<R: DieRoller>(pool: &DicePool, roller: &mut R) -> Result<RolledDicePool, BotError> {
-    let rolls: Vec<u32> = (0..pool.count)
+    let num_dice = calculate_dice_amount(&pool.amounts)?;
-        .flat_map(|_| roll_die(roller, pool))
+    let rolls: Vec<i32> = (0..num_dice).flat_map(|_| roll_die(roller, pool)).collect();
-        .collect();
+    Ok(RolledDicePool::from(pool, num_dice, rolls))
-
-    DicePoolRoll {
-        quality: pool.quality,
-        rolls: rolls,
-        exceptional_on: pool.exceptional_success,
-        success_on: pool.success_on,
-    }
 }
 
 #[cfg(test)]
@@ -247,12 +360,12 @@ mod tests {
     ///Instead of being random, generate a series of numbers we have complete
     ///control over.
     struct SequentialDieRoller {
-        results: Vec<u32>,
+        results: Vec<i32>,
         position: usize,
     }
 
     impl SequentialDieRoller {
-        fn new(results: Vec<u32>) -> SequentialDieRoller {
+        fn new(results: Vec<i32>) -> SequentialDieRoller {
             SequentialDieRoller {
                 results: results,
                 position: 0,
@@ -261,7 +374,7 @@ mod tests {
     }
 
     impl DieRoller for SequentialDieRoller {
-        fn roll_number(&mut self, _sides: u32) -> u32 {
+        fn roll_number(&mut self, _sides: i32) -> i32 {
             let roll = self.results[self.position];
             self.position += 1;
             roll
@@ -271,16 +384,18 @@ mod tests {
     //Sanity checks
     #[test]
     pub fn chance_die_has_success_on_10_test() {
-        assert_eq!(
+        assert_eq!(10, DicePool::chance_die().modifiers.success_on);
-            10,
-            DicePool::new(1, 5, DicePoolQuality::ChanceDie).success_on
-        );
     }
 
     #[test]
     pub fn non_chance_die_has_success_on_8_test() {
         fn check_success_on(quality: DicePoolQuality) {
-            assert_eq!(8, DicePool::new(1, 5, quality).success_on);
+            let modifiers = DicePoolModifiers::custom_quality(quality);
+            let amount = vec![Amount {
+                operator: Operator::Plus,
+                element: Element::Number(1),
+            }];
+            assert_eq!(8, DicePool::new(amount, modifiers).modifiers.success_on);
         }
 
         check_success_on(DicePoolQuality::TenAgain);
@@ -346,22 +461,73 @@ mod tests {
         assert_eq!(vec![8, 7], rolls);
     }
 
+    #[test]
+    fn dice_pool_modifiers_chance_die_test() {
+        let modifiers = DicePoolModifiers::custom_quality(DicePoolQuality::ChanceDie);
+        assert_eq!(10, modifiers.success_on);
+    }
+
+    #[test]
+    fn dice_pool_modifiers_default_sanity_check() {
+        let modifiers = DicePoolModifiers::default();
+        assert_eq!(8, modifiers.success_on);
+        assert_eq!(5, modifiers.exceptional_on);
+        assert_eq!(DicePoolQuality::TenAgain, modifiers.quality);
+    }
+
     #[test]
     pub fn no_explode_roll_test() {
-        let pool = DicePool::new(1, 5, DicePoolQuality::NoExplode);
+        let pool = DicePool::easy_pool(1, DicePoolQuality::NoExplode);
         let mut roller = SequentialDieRoller::new(vec![10, 8]);
-        let roll: DicePoolRoll = roll_dice(&pool, &mut roller);
+        let result = roll_dice(&pool, &mut roller);
+        assert!(result.is_ok());
+
+        let roll = result.unwrap().roll;
         assert_eq!(vec![10], roll.rolls());
     }
 
+    #[test]
+    pub fn number_of_dice_equality_test() {
+        let pool = DicePool::easy_pool(5, DicePoolQuality::NoExplode);
+        let mut roller = SequentialDieRoller::new(vec![1, 2, 3, 4, 5]);
+        let result = roll_dice(&pool, &mut roller);
+        assert!(result.is_ok());
+
+        let roll = result.unwrap();
+        assert_eq!(5, roll.num_dice);
+    }
+
     //DicePool tests
+    #[test]
+    fn easy_pool_chance_die_test() {
+        let pool = DicePool::easy_pool(1, DicePoolQuality::ChanceDie);
+        assert_eq!(10, pool.modifiers.success_on);
+    }
+
+    #[test]
+    fn easy_pool_quality_test() {
+        fn check_quality(quality: DicePoolQuality) {
+            let pool = DicePool::easy_pool(1, quality);
+            assert_eq!(quality, pool.modifiers.quality);
+        }
+
+        check_quality(DicePoolQuality::TenAgain);
+        check_quality(DicePoolQuality::NineAgain);
+        check_quality(DicePoolQuality::EightAgain);
+        check_quality(DicePoolQuality::Rote);
+        check_quality(DicePoolQuality::ChanceDie);
+        check_quality(DicePoolQuality::NoExplode);
+    }
+
     #[test]
     fn is_successful_on_equal_test() {
         let result = DicePoolRoll {
-            quality: DicePoolQuality::TenAgain,
             rolls: vec![8],
+            modifiers: DicePoolModifiers {
                 exceptional_on: 5,
                 success_on: 8,
+                quality: DicePoolQuality::TenAgain,
+            },
         };
 
         assert_eq!(1, result.successes());
@@ -370,10 +536,12 @@ mod tests {
     #[test]
     fn chance_die_success_test() {
         let result = DicePoolRoll {
-            quality: DicePoolQuality::TenAgain,
             rolls: vec![10],
+            modifiers: DicePoolModifiers {
                 exceptional_on: 5,
                 success_on: 10,
+                quality: DicePoolQuality::ChanceDie,
+            },
         };
 
         assert_eq!(1, result.successes());
@@ -382,10 +550,12 @@ mod tests {
     #[test]
     fn chance_die_fail_test() {
         let result = DicePoolRoll {
-            quality: DicePoolQuality::TenAgain,
             rolls: vec![9],
+            modifiers: DicePoolModifiers {
                 exceptional_on: 5,
                 success_on: 10,
+                quality: DicePoolQuality::ChanceDie,
+            },
         };
 
         assert_eq!(0, result.successes());
@@ -394,10 +564,12 @@ mod tests {
     #[test]
     fn is_exceptional_test() {
         let result = DicePoolRoll {
-            quality: DicePoolQuality::TenAgain,
             rolls: vec![8, 8, 9, 10, 8],
+            modifiers: DicePoolModifiers {
                 exceptional_on: 5,
                 success_on: 8,
+                quality: DicePoolQuality::TenAgain,
+            },
         };
 
         assert_eq!(5, result.successes());
@@ -407,10 +579,8 @@ mod tests {
     #[test]
     fn is_not_exceptional_test() {
         let result = DicePoolRoll {
-            quality: DicePoolQuality::TenAgain,
             rolls: vec![8, 8, 9, 10],
-            exceptional_on: 5,
+            modifiers: DicePoolModifiers::default(),
-            success_on: 8,
         };
 
         assert_eq!(4, result.successes());
@@ -421,10 +591,8 @@ mod tests {
     #[test]
     fn formats_dramatic_failure_test() {
         let result = DicePoolRoll {
-            quality: DicePoolQuality::ChanceDie,
             rolls: vec![1],
-            exceptional_on: 5,
+            modifiers: DicePoolModifiers::custom_quality(DicePoolQuality::ChanceDie),
-            success_on: 10,
         };
 
         assert_eq!("dramatic failure!", fmt_for_failure(&result));
@@ -433,10 +601,12 @@ mod tests {
     #[test]
     fn formats_regular_failure_when_not_chance_die_test() {
         let result = DicePoolRoll {
-            quality: DicePoolQuality::TenAgain,
             rolls: vec![1],
+            modifiers: DicePoolModifiers {
+                quality: DicePoolQuality::TenAgain,
                 exceptional_on: 5,
                 success_on: 10,
+            },
         };
 
         assert_eq!("failure!", fmt_for_failure(&result));
@@ -445,10 +615,8 @@ mod tests {
     #[test]
     fn formats_lots_of_dice_test() {
         let result = DicePoolRoll {
-            quality: DicePoolQuality::TenAgain,
+            modifiers: DicePoolModifiers::default(),
             rolls: vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 1, 2, 3, 4, 5, 6, 7, 8, 9],
-            exceptional_on: 5,
-            success_on: 10,
         };
 
         assert_eq!(

src/cofd/parser.rs

@@ -1,124 +1,205 @@
-use nom::{
+use crate::cofd::dice::{Amount, DicePool, DicePoolModifiers, DicePoolQuality, Element, Operator};
-    alt, bytes::complete::tag, character::complete::digit1, complete, many0, named,
+use crate::error::BotError;
-    sequence::tuple, tag, IResult,
+use combine::error::StringStreamError;
-};
+use combine::parser::char::{digit, letter, spaces, string};
+use combine::{choice, count, many, many1, one_of, Parser};
 
-use crate::cofd::dice::{DicePool, DicePoolQuality};
+#[derive(Debug, Clone, Copy, PartialEq)]
-use crate::parser::eat_whitespace;
+enum ParsedInfo {
-
+    Quality(DicePoolQuality),
-#[derive(Debug, PartialEq, Eq, Clone)]
+    ExceptionalOn(i32),
-pub enum DicePoolElement {
-    NumberOfDice(u32),
-    SuccessesForExceptional(u32),
-    DicePoolQuality(DicePoolQuality),
 }
 
-// Parse a single digit expression.  Does not eat whitespace
+#[derive(Debug, Clone, Copy, PartialEq)]
-fn parse_digit(input: &str) -> IResult<&str, u32> {
+pub enum DiceParsingError {
-    let (input, num) = digit1(input)?;
+    InvalidAmount,
-    Ok((input, num.parse().unwrap()))
+    InvalidModifiers,
+    UnconsumedInput,
 }
 
-fn parse_quality(input: &str) -> IResult<&str, DicePoolQuality> {
+impl std::fmt::Display for DiceParsingError {
-    let (input, _) = eat_whitespace(input)?;
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-    named!(quality(&str) -> DicePoolQuality, alt!(
+        write!(f, "{}", self.as_str())
-        complete!(tag!("n")) => { |_| DicePoolQuality::NineAgain } |
+    }
-        complete!(tag!("e")) => { |_| DicePoolQuality::EightAgain } |
-        complete!(tag!("r")) => { |_| DicePoolQuality::Rote } |
-        complete!(tag!("x")) => { |_| DicePoolQuality::NoExplode }
-    ));
-
-    let (input, dice_pool_quality) = quality(input)?;
-    Ok((input, dice_pool_quality))
 }
 
-fn parse_exceptional_requirement(input: &str) -> IResult<&str, u32> {
+impl std::error::Error for DiceParsingError {
-    let (input, _) = eat_whitespace(input)?;
+    fn description(&self) -> &str {
-    let (input, (_, successes)) = tuple((tag("s"), digit1))(input)?;
+        self.as_str()
-    Ok((input, successes.parse().unwrap()))
+    }
 }
 
-// Parse a dice pool element expression.  Eats whitespace.
+impl DiceParsingError {
-fn parse_dice_pool_element(input: &str) -> IResult<&str, DicePoolElement> {
+    fn as_str(&self) -> &str {
-    let (input, _) = eat_whitespace(input)?;
+        use self::DiceParsingError::*;
-    named!(element(&str) -> DicePoolElement, alt!(
+        match *self {
-        parse_digit => { |num| DicePoolElement::NumberOfDice(num) } |
+            InvalidAmount => "invalid amount of dice",
-        parse_quality => { |qual| DicePoolElement::DicePoolQuality(qual) } |
+            InvalidModifiers => "dice pool modifiers not specified properly",
-        parse_exceptional_requirement => { |succ| DicePoolElement::SuccessesForExceptional(succ) }
+            UnconsumedInput => "extraneous input detected",
-    ));
+        }
-
+    }
-    let (input, element) = element(input)?;
-    Ok((input, element))
 }
 
-fn find_elements(elements: Vec<DicePoolElement>) -> (Option<u32>, DicePoolQuality, u32) {
+pub fn parse_modifiers(input: &str) -> Result<DicePoolModifiers, BotError> {
-    let mut found_quality: Option<DicePoolQuality> = None;
+    if input.len() == 0 {
-    let mut found_count: Option<u32> = None;
+        return Ok(DicePoolModifiers::default());
-    let mut found_successes_required: Option<u32> = None;
-
-    for element in elements {
-        if found_quality.is_some() && found_count.is_some() && found_successes_required.is_some() {
-            break;
     }
 
-        match element {
+    let input = input.trim();
-            DicePoolElement::NumberOfDice(found) => {
-                if found_count.is_none() {
-                    found_count = Some(found);
-                }
-            }
-            DicePoolElement::DicePoolQuality(found) => {
-                if found_quality.is_none() {
-                    found_quality = Some(found);
-                }
-            }
-            DicePoolElement::SuccessesForExceptional(found) => {
-                if found_successes_required.is_none() {
-                    found_successes_required = Some(found);
-                }
-            }
-        };
-    }
 
-    let quality: DicePoolQuality = found_quality.unwrap_or(DicePoolQuality::TenAgain);
+    let quality = one_of("nerx".chars())
-    let successes_for_exceptional: u32 = found_successes_required.unwrap_or(5);
+        .skip(spaces().silent())
-    (found_count, quality, successes_for_exceptional)
+        .map(|quality| match quality {
-}
+            'n' => ParsedInfo::Quality(DicePoolQuality::NineAgain),
+            'e' => ParsedInfo::Quality(DicePoolQuality::EightAgain),
+            'r' => ParsedInfo::Quality(DicePoolQuality::Rote),
+            'x' => ParsedInfo::Quality(DicePoolQuality::NoExplode),
+            _ => ParsedInfo::Quality(DicePoolQuality::TenAgain), //TODO add warning log
+        });
 
-fn convert_to_dice_pool(input: &str, elements: Vec<DicePoolElement>) -> IResult<&str, DicePool> {
+    let exceptional_on = string("s")
-    let (count, quality, successes_for_exceptional) = find_elements(elements);
+        .and(many1(digit()))
+        .map(|s| s.1) //Discard the s; only need the number
+        .skip(spaces().silent())
+        .map(|num_as_str: String| {
+            ParsedInfo::ExceptionalOn(match num_as_str.parse::<i32>() {
+                Ok(success_on) => success_on,
+                Err(_) => 5, //TODO add warning log
+            })
+        });
 
-    if count.is_some() {
+    let mut parser = count(2, choice((quality, exceptional_on)))
-        Ok((
+        .skip(spaces().silent())
-            input,
+        .map(|modifiers: Vec<ParsedInfo>| modifiers);
-            DicePool::new(count.unwrap(), successes_for_exceptional, quality),
+
+    let (result, rest) = parser.parse(input)?;
+
+    if rest.len() == 0 {
+        convert_to_info(&result)
+    } else {
+        Err(BotError::DiceParsingError(
+            DiceParsingError::UnconsumedInput,
         ))
+    }
+}
+
+fn convert_to_info(parsed: &Vec<ParsedInfo>) -> Result<DicePoolModifiers, BotError> {
+    use ParsedInfo::*;
+    if parsed.len() == 0 {
+        Ok(DicePoolModifiers::default())
+    } else if parsed.len() == 1 {
+        match parsed[0] {
+            ExceptionalOn(exceptional_on) => {
+                Ok(DicePoolModifiers::custom_exceptional_on(exceptional_on))
+            }
+            Quality(quality) => Ok(DicePoolModifiers::custom_quality(quality)),
+        }
+    } else if parsed.len() == 2 {
+        match parsed[..] {
+            [ExceptionalOn(exceptional_on), Quality(quality)] => {
+                Ok(DicePoolModifiers::custom(quality, exceptional_on))
+            }
+            [Quality(quality), ExceptionalOn(exceptional_on)] => {
+                Ok(DicePoolModifiers::custom(quality, exceptional_on))
+            }
+            _ => Err(DiceParsingError::InvalidModifiers.into()),
+        }
     } else {
-        use nom::error::ErrorKind;
+        //We don't expect this clause to be hit, because the parser works 0 to 2 times.
-        use nom::Err;
+        Err(DiceParsingError::InvalidModifiers.into())
-        Err(Err::Error((input, ErrorKind::Alt)))
     }
 }
 
-pub fn parse_dice_pool(input: &str) -> IResult<&str, DicePool> {
+/// Parse dice pool amounts into elements coupled with operators,
-    named!(first_element(&str) -> DicePoolElement, alt!(
+/// where an operator is "+" or "-", and an element is either a number
-            parse_dice_pool_element => { |e| e }
+/// or variable name. The first element should not have an operator,
-    ));
+/// but every one after that should. Accepts expressions like "8", "10
-    let (input, first) = first_element(input)?;
+/// + variablename", "variablename - 3", etc.
-    let (input, elements) = if input.trim().is_empty() {
+fn parse_pool_amount(input: &str) -> Result<Vec<Amount>, BotError> {
-        (input, vec![first])
+    let input = input.trim();
+
+    let plus_or_minus = one_of("+-".chars());
+    let maybe_sign = plus_or_minus.map(|sign: char| match sign {
+        '+' => Operator::Plus,
+        '-' => Operator::Minus,
+        _ => Operator::Plus,
+    });
+
+    //TODO make this a macro or something
+    let first = many1(letter())
+        .or(many1(digit()))
+        .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)| {
+            let mut amounts = vec![(results.0).0];
+            amounts.append(&mut (results.0).1);
+            (amounts, results.1)
+        })?;
+
+    if rest.len() == 0 {
+        Ok(results)
     } else {
-        named!(rest_elements(&str) -> Vec<DicePoolElement>, many0!(parse_dice_pool_element));
+        Err(BotError::DiceParsingError(
-        let (input, mut rest) = rest_elements(input)?;
+            DiceParsingError::UnconsumedInput,
-        rest.insert(0, first);
+        ))
-        (input, rest)
+    }
-    };
-
-    convert_to_dice_pool(input, elements)
 }
 
-pub fn create_chance_die() -> IResult<&'static str, DicePool> {
+pub fn parse_dice_pool(input: &str) -> Result<DicePool, BotError> {
-    Ok(("", DicePool::chance_die()))
+    //The "modifiers:" part is optional. Assume amounts if no modifier
+    //section found.
+    let split = input.split(":").collect::<Vec<_>>();
+    let (modifiers_str, amounts_str) = (match split[..] {
+        [amounts] => Ok(("", amounts)),
+        [modifiers, amounts] => Ok((modifiers, amounts)),
+        _ => Err(BotError::DiceParsingError(
+            DiceParsingError::UnconsumedInput,
+        )),
+    })?;
+
+    let modifiers = parse_modifiers(modifiers_str)?;
+    let amounts = parse_pool_amount(&amounts_str)?;
+    Ok(DicePool::new(amounts, modifiers))
+}
+
+pub fn create_chance_die() -> Result<DicePool, StringStreamError> {
+    Ok(DicePool::chance_die())
 }
 
 #[cfg(test)]
@@ -126,109 +207,206 @@ mod tests {
     use super::*;
 
     #[test]
-    fn parse_digit_test() {
+    fn parse_single_number_amount_test() {
-        use nom::error::ErrorKind;
+        let result = parse_pool_amount("1");
-        use nom::Err;
+        assert!(result.is_ok());
-        assert_eq!(parse_digit("1"), Ok(("", 1)));
-        assert_eq!(parse_digit("10"), Ok(("", 10)));
         assert_eq!(
-            parse_digit("adsf"),
+            result.unwrap(),
-            Err(Err::Error(("adsf", ErrorKind::Digit)))
+            vec![Amount {
+                operator: Operator::Plus,
+                element: Element::Number(1)
+            }]
         );
+
+        let result = parse_pool_amount("10");
+        assert!(result.is_ok());
+        assert_eq!(
+            result.unwrap(),
+            vec![Amount {
+                operator: Operator::Plus,
+                element: Element::Number(10)
+            }]
+        );
+    }
+
+    #[test]
+    fn parse_single_variable_amount_test() {
+        let result = parse_pool_amount("asdf");
+        assert!(result.is_ok());
+        assert_eq!(
+            result.unwrap(),
+            vec![Amount {
+                operator: Operator::Plus,
+                element: Element::Variable("asdf".to_string())
+            }]
+        );
+
+        let result = parse_pool_amount("nosis");
+        assert!(result.is_ok());
+        assert_eq!(
+            result.unwrap(),
+            vec![Amount {
+                operator: Operator::Plus,
+                element: Element::Variable("nosis".to_string())
+            }]
+        );
+    }
+
+    #[test]
+    fn parse_complex_amount_expression() {
+        assert!(parse_pool_amount("1 + myvariable - 2").is_ok());
     }
 
     #[test]
     fn quality_test() {
-        use nom::error::ErrorKind;
+        let result = parse_modifiers("n");
-        use nom::Err;
+        assert!(result.is_ok());
-        assert_eq!(parse_quality("n"), Ok(("", DicePoolQuality::NineAgain)));
+        assert_eq!(
-        assert_eq!(parse_quality("e"), Ok(("", DicePoolQuality::EightAgain)));
+            result.unwrap(),
-        assert_eq!(parse_quality("r"), Ok(("", DicePoolQuality::Rote)));
+            DicePoolModifiers::custom_quality(DicePoolQuality::NineAgain)
-        assert_eq!(parse_quality("x"), Ok(("", DicePoolQuality::NoExplode)));
+        );
-        assert_eq!(parse_quality("b"), Err(Err::Error(("b", ErrorKind::Alt))));
+
+        let result = parse_modifiers("e");
+        assert!(result.is_ok());
+        assert_eq!(
+            result.unwrap(),
+            DicePoolModifiers::custom_quality(DicePoolQuality::EightAgain)
+        );
+
+        let result = parse_modifiers("r");
+        assert!(result.is_ok());
+        assert_eq!(
+            result.unwrap(),
+            DicePoolModifiers::custom_quality(DicePoolQuality::Rote)
+        );
+
+        let result = parse_modifiers("x");
+        assert!(result.is_ok());
+        assert_eq!(
+            result.unwrap(),
+            DicePoolModifiers::custom_quality(DicePoolQuality::NoExplode)
+        );
+
+        let result = parse_modifiers("b");
+        assert!(result.is_err());
+        assert!(matches!(
+            result,
+            Err(BotError::DiceParsingError(
+                DiceParsingError::UnconsumedInput
+            ))
+        ));
     }
 
     #[test]
-    fn multiple_quality_test() {
+    fn multiple_quality_failure_test() {
-        assert_eq!(parse_quality("ner"), Ok(("er", DicePoolQuality::NineAgain)));
+        let result = parse_modifiers("ne");
+        assert!(result.is_err());
+        assert!(matches!(
+            result,
+            Err(BotError::DiceParsingError(
+                DiceParsingError::InvalidModifiers
+            ))
+        ));
     }
 
     #[test]
     fn exceptional_success_test() {
-        use nom::error::ErrorKind;
+        let result = parse_modifiers("s3");
-        use nom::Err;
+        assert!(result.is_ok());
-        assert_eq!(parse_exceptional_requirement("s3"), Ok(("", 3)));
+        assert_eq!(result.unwrap(), DicePoolModifiers::custom_exceptional_on(3));
-        assert_eq!(parse_exceptional_requirement("s10"), Ok(("", 10)));
-        assert_eq!(parse_exceptional_requirement("s20b"), Ok(("b", 20)));
-        assert_eq!(
-            parse_exceptional_requirement("sab10"),
-            Err(Err::Error(("ab10", ErrorKind::Digit)))
-        );
-    }
-
-    #[test]
-    fn dice_pool_element_expression_test() {
-        use nom::error::ErrorKind;
-        use nom::Err;
 
+        let result = parse_modifiers("s33");
+        assert!(result.is_ok());
         assert_eq!(
-            parse_dice_pool_element("8"),
+            result.unwrap(),
-            Ok(("", DicePoolElement::NumberOfDice(8)))
+            DicePoolModifiers::custom_exceptional_on(33)
         );
 
-        assert_eq!(
+        let result = parse_modifiers("s3q");
-            parse_dice_pool_element("n"),
+        assert!(result.is_err());
-            Ok((
+        assert!(matches!(
-                "",
+            result,
-                DicePoolElement::DicePoolQuality(DicePoolQuality::NineAgain)
+            Err(BotError::DiceParsingError(
+                DiceParsingError::UnconsumedInput
             ))
-        );
+        ));
-
-        assert_eq!(
-            parse_dice_pool_element("s3"),
-            Ok(("", DicePoolElement::SuccessesForExceptional(3)))
-        );
-
-        assert_eq!(
-            parse_dice_pool_element("8ns3"),
-            Ok(("ns3", DicePoolElement::NumberOfDice(8)))
-        );
-
-        assert_eq!(
-            parse_dice_pool_element("totallynotvalid"),
-            Err(Err::Error(("totallynotvalid", ErrorKind::Alt)))
-        );
     }
 
     #[test]
     fn dice_pool_number_only_test() {
+        let result = parse_dice_pool("8");
+        assert!(result.is_ok());
         assert_eq!(
-            parse_dice_pool("8"),
+            result.unwrap(),
-            Ok(("", DicePool::new(8, 5, DicePoolQuality::TenAgain)))
+            DicePool::easy_pool(8, DicePoolQuality::TenAgain)
         );
     }
 
     #[test]
     fn dice_pool_number_with_quality() {
+        let result = parse_dice_pool("n:8");
+        assert!(result.is_ok());
         assert_eq!(
-            parse_dice_pool("8n"),
+            result.unwrap(),
-            Ok(("", DicePool::new(8, 5, DicePoolQuality::NineAgain)))
+            DicePool::easy_pool(8, DicePoolQuality::NineAgain)
         );
     }
 
     #[test]
     fn dice_pool_number_with_success_change() {
-        assert_eq!(
+        let modifiers = DicePoolModifiers::custom_exceptional_on(3);
-            parse_dice_pool("8s3"),
+        let result = parse_dice_pool("s3:8");
-            Ok(("", DicePool::new(8, 3, DicePoolQuality::TenAgain)))
+        assert!(result.is_ok());
-        );
+        assert_eq!(result.unwrap(), DicePool::easy_with_modifiers(8, modifiers));
     }
 
     #[test]
     fn dice_pool_with_quality_and_success_change() {
-        assert_eq!(
+        let modifiers = DicePoolModifiers::custom(DicePoolQuality::Rote, 3);
-            parse_dice_pool("8rs3"),
+        let result = parse_dice_pool("rs3:8");
-            Ok(("", DicePool::new(8, 3, DicePoolQuality::Rote)))
+        assert!(result.is_ok());
-        );
+        assert_eq!(result.unwrap(), DicePool::easy_with_modifiers(8, modifiers));
+    }
+
+    #[test]
+    fn dice_pool_complex_expression_test() {
+        let modifiers = DicePoolModifiers::custom(DicePoolQuality::Rote, 3);
+        let amounts = vec![
+            Amount {
+                operator: Operator::Plus,
+                element: Element::Number(8),
+            },
+            Amount {
+                operator: Operator::Plus,
+                element: Element::Number(10),
+            },
+            Amount {
+                operator: Operator::Minus,
+                element: Element::Number(2),
+            },
+            Amount {
+                operator: Operator::Plus,
+                element: Element::Variable("varname".to_owned()),
+            },
+        ];
+
+        let expected = DicePool::new(amounts, modifiers);
+
+        let result = parse_dice_pool("rs3:8+10-2+varname");
+        assert!(result.is_ok());
+        assert_eq!(result.unwrap(), expected);
+
+        let result = parse_dice_pool("rs3:8+10-   2 + varname");
+        assert!(result.is_ok());
+        assert_eq!(result.unwrap(), expected);
+
+        let result = parse_dice_pool("rs3  :  8+ 10 -2 + varname");
+        assert!(result.is_ok());
+        assert_eq!(result.unwrap(), expected);
+
+        //This one has tabs in it.
+        let result = parse_dice_pool("  r	s3  :	8	+ 10 -2 + varname");
+        assert!(result.is_ok());
+        assert_eq!(result.unwrap(), expected);
     }
 }

src/commands.rs

@@ -1,5 +1,6 @@
 use crate::cofd::dice::DicePool;
 use crate::dice::ElementExpression;
+use crate::error::BotError;
 use crate::help::HelpTopic;
 use crate::roll::Roll;
 
@@ -51,12 +52,24 @@ impl Command for PoolRollCommand {
     }
 
     fn execute(&self) -> Execution {
-        let roll = self.0.roll();
+        let roll_result = self.0.roll();
-        let plain = format!("Pool: {}\nResult: {}", self.0, roll);
+
+        let (plain, html) = match roll_result {
+            Ok(rolled_pool) => {
+                let plain = format!("Pool: {}\nResult: {}", rolled_pool, rolled_pool.roll);
                 let html = format!(
                     "<p><strong>Pool:</strong> {}</p><p><strong>Result</strong>: {}</p>",
-            self.0, roll
+                    rolled_pool, rolled_pool.roll
                 );
+                (plain, html)
+            }
+            Err(e) => {
+                let plain = format!("Error: {}", e);
+                let html = format!("<p><strong>Error:</strong> {}</p>", e);
+                (plain, html)
+            }
+        };
+
         Execution { plain, html }
     }
 }
@@ -83,20 +96,21 @@ impl Command for HelpCommand {
 /// Parse a command string into a dynamic command execution trait
 /// object. Returns an error if a command was recognized but not
 /// parsed correctly. Returns Ok(None) if no command was recognized.
-pub fn parse_command(s: &str) -> Result<Option<Box<dyn Command>>, String> {
+pub fn parse_command(s: &str) -> Result<Option<Box<dyn Command>>, BotError> {
-    match parser::parse_command(s) {
+    // match parser::parse_command(s) {
-        Ok((input, command)) => match (input, &command) {
+    //     Ok(Some(command)) => match &command {
-            //Any command, or text transformed into non-command is
+    //         //Any command, or text transformed into non-command is
-            //sent upwards.
+    //         //sent upwards.
-            ("", Some(_)) | (_, None) => Ok(command),
+    //         ("", Some(_)) | (_, None) => Ok(command),
 
-            //TODO replcae with nom all_consuming?
+    //         //TODO replcae with nom all_consuming?
-            //Any unconsumed input (whitespace should already be
+    //         //Any unconsumed input (whitespace should already be
-            // stripped) is considered a parsing error.
+    //         // stripped) is considered a parsing error.
-            _ => Err(format!("{}: malformed expression", s)),
+    //         _ => Err(format!("{}: malformed expression", s)),
-        },
+    //     },
-        Err(err) => Err(err.to_string()),
+    //     Err(err) => Err(err),
-    }
+    // }
+    parser::parse_command(s)
 }
 
 #[cfg(test)]
@@ -122,13 +136,13 @@ mod tests {
 
     #[test]
     fn pool_whitespace_test() {
-        assert!(parse_command("!pool 8ns3   ")
+        assert!(parse_command("!pool ns3:8   ")
             .map(|p| p.is_some())
             .expect("was error"));
-        assert!(parse_command("   !pool 8ns3")
+        assert!(parse_command("   !pool ns3:8")
             .map(|p| p.is_some())
             .expect("was error"));
-        assert!(parse_command("   !pool 8ns3   ")
+        assert!(parse_command("   !pool ns3:8   ")
             .map(|p| p.is_some())
             .expect("was error"));
     }

src/commands/parser.rs

@@ -1,68 +1,84 @@
 use crate::cofd::parser::{create_chance_die, parse_dice_pool};
 use crate::commands::{Command, HelpCommand, PoolRollCommand, RollCommand};
 use crate::dice::parser::parse_element_expression;
+use crate::error::BotError;
 use crate::help::parse_help_topic;
-use nom::bytes::streaming::tag;
+use combine::parser::char::{char, letter, space};
-use nom::error::ErrorKind as NomErrorKind;
+use combine::{any, many1, optional, Parser};
 use nom::Err as NomErr;
-use nom::{character::complete::alpha1, IResult};
 
 // Parse a roll expression.
-fn parse_roll(input: &str) -> IResult<&str, Box<dyn Command>> {
+fn parse_roll(input: &str) -> Result<Box<dyn Command>, BotError> {
-    let (input, expression) = parse_element_expression(input)?;
+    let result = parse_element_expression(input);
-    Ok((input, Box::new(RollCommand(expression))))
+    match result {
+        Ok((rest, expression)) if rest.len() == 0 => Ok(Box::new(RollCommand(expression))),
+        //Legacy code boundary translates nom errors into BotErrors.
+        Ok(_) => Err(BotError::NomParserIncomplete),
+        Err(NomErr::Error(e)) => Err(BotError::NomParserError(e.1)),
+        Err(NomErr::Failure(e)) => Err(BotError::NomParserError(e.1)),
+        Err(NomErr::Incomplete(_)) => Err(BotError::NomParserIncomplete),
+    }
 }
 
-fn parse_pool_roll(input: &str) -> IResult<&str, Box<dyn Command>> {
+fn parse_pool_roll(input: &str) -> Result<Box<dyn Command>, BotError> {
-    let (input, pool) = parse_dice_pool(input)?;
+    let pool = parse_dice_pool(input)?;
-    Ok((input, Box::new(PoolRollCommand(pool))))
+    Ok(Box::new(PoolRollCommand(pool)))
 }
 
-fn chance_die() -> IResult<&'static str, Box<dyn Command>> {
+fn chance_die() -> Result<Box<dyn Command>, BotError> {
-    let (input, pool) = create_chance_die()?;
+    let pool = create_chance_die()?;
-    Ok((input, Box::new(PoolRollCommand(pool))))
+    Ok(Box::new(PoolRollCommand(pool)))
 }
 
-fn help(topic: &str) -> IResult<&str, Box<dyn Command>> {
+fn help(topic: &str) -> Result<Box<dyn Command>, BotError> {
     let topic = parse_help_topic(topic);
-    Ok(("", Box::new(HelpCommand(topic))))
+    Ok(Box::new(HelpCommand(topic)))
 }
 
 /// Split an input string into its constituent command and "everything
 /// else" parts. Extracts the command separately from its input (i.e.
 /// rest of the line) and returns a tuple of (command_input, command).
 /// Whitespace at the start and end of the command input is removed.
-fn split_command(input: &str) -> IResult<&str, &str> {
+fn split_command(input: &str) -> Result<(String, String), BotError> {
-    let input = input.trim_start();
+    let input = input.trim();
-    let (input, _) = tag("!")(input)?;
 
-    let (mut command_input, command) = alpha1(input)?;
+    let exclamation = char('!');
-    command_input = command_input.trim();
+    let word = many1(letter()).map(|value: String| value);
-    Ok((command_input, command))
+    let at_least_one_space = many1(space().silent()).map(|value: String| value);
+    let cmd_input = optional(at_least_one_space.and(many1(any()).map(|value: String| value)));
+
+    let mut parser = exclamation.and(word).and(cmd_input);
+
+    //TODO make less wacky, possibly by mapping it into a struct and
+    // making use of skip. This super-wacky tuple is:
+    //  (parsed_input, rest)
+    //Where parsed_input is:
+    //  (!command, option<arguments>)
+    //Where !command is:
+    //  ('!', command)
+    //Were option<arguments> is:
+    // Option tuple of (whitespace, arguments)
+    let (command, command_input) = match parser.parse(input)? {
+        (((_, command), Some((_, command_input))), _) => (command, command_input),
+        (((_, command), None), _) => (command, "".to_string()),
+    };
+
+    Ok((command, command_input))
 }
 
 /// Potentially parse a command expression. If we recognize the
 /// command, an error should be raised if the command is misparsed. If
 /// we don't recognize the command, ignore it and return None.
-pub fn parse_command(input: &str) -> IResult<&str, Option<Box<dyn Command>>> {
+pub fn parse_command(input: &str) -> Result<Option<Box<dyn Command>>, BotError> {
     match split_command(input) {
-        Ok((cmd_input, cmd)) => match cmd {
+        Ok((cmd, cmd_input)) => match cmd.as_ref() {
-            "r" | "roll" => parse_roll(cmd_input).map(|(input, command)| (input, Some(command))),
+            "r" | "roll" => parse_roll(&cmd_input).map(|command| Some(command)),
-            "rp" | "pool" => {
+            "rp" | "pool" => parse_pool_roll(&cmd_input).map(|command| Some(command)),
-                parse_pool_roll(cmd_input).map(|(input, command)| (input, Some(command)))
+            "chance" => chance_die().map(|command| Some(command)),
-            }
+            "help" => help(&cmd_input).map(|command| Some(command)),
-            "chance" => chance_die().map(|(input, command)| (input, Some(command))),
-            "help" => help(cmd_input).map(|(input, command)| (input, Some(command))),
             // No recognized command, ignore this.
-            _ => Ok((input, None)),
+            _ => Ok(None),
         },
-
-        //TODO better way to do this?
-        //If the input is not a command, or the message is incomplete
-        //(empty), we declare this to be a non-command, and don't do
-        //anything else with it.
-        Err(NomErr::Error((_, NomErrorKind::Tag))) | Err(NomErr::Incomplete(_)) => Ok(("", None)),
-
         //All other errors passed up.
         Err(e) => Err(e),
     }
@@ -72,18 +88,19 @@ pub fn parse_command(input: &str) -> IResult<&str, Option<Box<dyn Command>>> {
 mod tests {
     use super::*;
 
+    //TODO these errors don't seem to implement the right traits to do
+    //eq checks or even unwrap_err!
+
     #[test]
     fn non_command_test() {
         let result = parse_command("not a command");
-        assert!(result.is_ok());
+        assert!(result.is_err());
-        assert!(result.unwrap().1.is_none());
     }
 
     #[test]
     fn empty_message_test() {
         let result = parse_command("");
-        assert!(result.is_ok());
+        assert!(result.is_err());
-        assert!(result.unwrap().1.is_none());
     }
 
     #[test]
@@ -95,22 +112,19 @@ mod tests {
     #[test]
     fn word_with_exclamation_mark_test() {
         let result1 = parse_command("hello !notacommand");
-        assert!(result1.is_ok());
+        assert!(result1.is_err());
-        assert!(result1.unwrap().1.is_none());
 
         let result2 = parse_command("hello!");
-        assert!(result2.is_ok());
+        assert!(result2.is_err());
-        assert!(result2.unwrap().1.is_none());
 
         let result3 = parse_command("hello!notacommand");
-        assert!(result3.is_ok());
+        assert!(result3.is_err());
-        assert!(result3.unwrap().1.is_none());
     }
 
     #[test]
     fn basic_command_test() {
         assert_eq!(
-            ("1d4", "roll"),
+            ("roll".to_string(), "1d4".to_string()),
             split_command("!roll 1d4").expect("got parsing error")
         );
     }
@@ -118,7 +132,7 @@ mod tests {
     #[test]
     fn whitespace_at_start_test() {
         assert_eq!(
-            ("1d4", "roll"),
+            ("roll".to_string(), "1d4".to_string()),
             split_command("   !roll 1d4").expect("got parsing error")
         );
     }
@@ -126,7 +140,7 @@ mod tests {
     #[test]
     fn whitespace_at_end_test() {
         assert_eq!(
-            ("1d4", "roll"),
+            ("roll".to_string(), "1d4".to_string()),
             split_command("!roll 1d4   ").expect("got parsing error")
         );
     }
@@ -134,7 +148,7 @@ mod tests {
     #[test]
     fn whitespace_on_both_ends_test() {
         assert_eq!(
-            ("1d4", "roll"),
+            ("roll".to_string(), "1d4".to_string()),
             split_command("   !roll 1d4   ").expect("got parsing error")
         );
     }
@@ -142,12 +156,12 @@ mod tests {
     #[test]
     fn single_command_test() {
         assert_eq!(
-            ("", "roll"),
+            ("roll".to_string(), "".to_string()),
             split_command("!roll").expect("got parsing error")
         );
 
         assert_eq!(
-            ("", "thisdoesnotexist"),
+            ("thisdoesnotexist".to_string(), "".to_string()),
             split_command("!thisdoesnotexist").expect("got parsing error")
         );
     }

src/error.rs

@@ -33,4 +33,19 @@ pub enum BotError {
 
     #[error("i/o error")]
     IoError(#[from] std::io::Error),
+
+    #[error("parsing error")]
+    ParserError(#[from] combine::error::StringStreamError),
+
+    #[error("dice parsing error")]
+    DiceParsingError(#[from] crate::cofd::parser::DiceParsingError),
+
+    #[error("legacy parsing error")]
+    NomParserError(nom::error::ErrorKind),
+
+    #[error("legacy parsing error: not enough data")]
+    NomParserIncomplete,
+
+    #[error("variables not yet supported")]
+    VariablesNotSupported,
 }