projectmoon
/
tenebrous-dicebot
1
0
Fork 1
Code Issues 12 Pull Requests 1 Projects Releases 10 Wiki Activity

Keep/Drop Function #92

Manually merged
kg333 merged 12 commits from kg333/tenebrous-dicebot:keep_drop_function into master 2021-09-26 14:05:57 +00:00
Conversation 2 Commits 12 Files Changed 4 +27 -42
3 changed files with 27 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 7e7e9e534e - Show all commits

18
dicebot/src/basic/dice.rs
View File

@ -18,20 +18,9 @@ pub struct Dice {
impl fmt::Display for Dice { impl fmt::Display for Dice {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self.keep_drop { match self.keep_drop {
KeepOrDrop::Keep(keep) => { KeepOrDrop::Keep(keep) => write!(f, "{}d{}k{}", self.count, self.sides, keep),
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-22 19:51:54 +00:00
Outdated
Review
Copy Link

Since you have if-else clauses here, it might make more sense to have a third enum member in addition to Keep and Drop, which indicates that we do nothing special with the roll.

Since you have if-else clauses here, it might make more sense to have a third enum member in addition to Keep and Drop, which indicates that we do nothing special with the roll.
kg333 commented 2021-09-25 03:04:43 +00:00
Outdated
Review
Copy Link

Added another enum member called None and it's much cleaner. Although members with no type give my C-programmer soul the heebie-jeebies.

Added another enum member called None and it's much cleaner. Although members with no type give my C-programmer soul the heebie-jeebies.
if keep != self.count { KeepOrDrop::Drop(drop) => write!(f, "{}d{}dh{}", self.count, self.sides, drop),
write!(f, "{}d{}k{}", self.count, self.sides, keep) KeepOrDrop::None => write!(f, "{}d{}", self.count, self.sides),
} else {
write!(f, "{}d{}", self.count, self.sides)
}
}
KeepOrDrop::Drop(drop) => {
if drop != 0 {
write!(f, "{}d{}dh{}", self.count, self.sides, drop)
} else {
write!(f, "{}d{}", self.count, self.sides)
}
}
} }
} }
} }
@ -40,6 +29,7 @@ impl fmt::Display for Dice {
pub enum KeepOrDrop { pub enum KeepOrDrop {
Keep (u32), Keep (u32),
Drop (u32), Drop (u32),
None,
} }
impl Dice { impl Dice {

50
dicebot/src/basic/parser.rs
View File

@ -39,7 +39,7 @@ fn parse_dice(input: &str) -> IResult<&str, Dice> {
// check for keep expression to keep highest dice (2d20k1) // check for keep expression to keep highest dice (2d20k1)
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-17 06:51:15 +00:00
Outdated
Review
Copy Link

I think it is better to document what the keep expression actually is here, so tacking on "1dXkY" would be useful.

And since pattern matches are expressions, I think we can rewrite these two match blocks. Something like this:

let (keep, input) = match tuple::<&str, _, (_, _), _>((tag("k"), digit1))(input) {
    // if ok, keep expression is present
    Ok(r) => (r.0, r.1.1),
    // otherwise absent and keep all dice
    Err(_) => ("".to_string(), count)
};

The same should be doable for the drop expression. I think this way you can also get rid of the mut on input, because let in rust is not just a variable assignment. It actually creates a whole new variable binding.

Note: I haven't tested the above, but it should work. Or some variation of it should work.

I think it is better to document what the keep expression actually is here, so tacking on "1dXkY" would be useful. And since pattern matches are expressions, I think we can rewrite these two match blocks. Something like this: ```rust let (keep, input) = match tuple::<&str, _, (_, _), _>((tag("k"), digit1))(input) { // if ok, keep expression is present Ok(r) => (r.0, r.1.1), // otherwise absent and keep all dice Err(_) => ("".to_string(), count) }; ``` The same should be doable for the drop expression. I think this way you can also get rid of the `mut` on input, because `let` in rust is not just a variable assignment. It actually creates a whole new variable binding. Note: I haven't tested the above, but it should work. Or some variation of it should work.
let (keep, input) = match tuple::<&str, _, (_, _), _>((tag("k"), digit1))(input) { let (keep, input) = match tuple::<&str, _, (_, _), _>((tag("k"), digit1))(input) {
// if ok, keep expression is present // if ok, keep expression is present
Ok(r) => (r.1.1, r.0), Ok((rest, (_, keep_amount))) => (keep_amount, rest),
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-22 19:53:33 +00:00
Outdated
Review
Copy Link

Rust supports tuple destructuring in pattern matching, which may be useful here and the other match:

Ok((rest, (_, keep_amount)) => ...

rest being the rest of the input in this case.

Rust supports tuple destructuring in pattern matching, which may be useful here and the other match: ```rust Ok((rest, (_, keep_amount)) => ... ``` `rest` being the rest of the input in this case.
kg333 commented 2021-09-25 02:58:04 +00:00
Outdated
Review
Copy Link

Ooh, that's much more human-readable, thanks!

Ooh, that's much more human-readable, thanks!
// otherwise absent and keep all dice // otherwise absent and keep all dice
Err(_) => ("", input) Err(_) => ("", input)
}; };
@ -47,7 +47,7 @@ fn parse_dice(input: &str) -> IResult<&str, Dice> {
// check for drop expression to drop highest dice (2d20dh1) // check for drop expression to drop highest dice (2d20dh1)
let (drop, input) = match tuple::<&str, _, (_, _), _>((tag("dh"), digit1))(input) { let (drop, input) = match tuple::<&str, _, (_, _), _>((tag("dh"), digit1))(input) {
// if ok, keep expression is present // if ok, keep expression is present
Ok(r) => (r.1.1, r.0), Ok((rest, (_, drop_amount))) => (drop_amount, rest),
// otherwise absent and keep all dice // otherwise absent and keep all dice
Err(_) => ("", input) Err(_) => ("", input)
}; };
@ -57,26 +57,20 @@ fn parse_dice(input: &str) -> IResult<&str, Dice> {
// don't allow keep greater than number of dice, and don't allow keep zero // don't allow keep greater than number of dice, and don't allow keep zero
let keep_drop = match keep.parse::<u32>() { let keep_drop = match keep.parse::<u32>() {
// Ok, there's a keep value, check and create Keep // Ok, there's a keep value, check and create Keep
Ok(i) => { Ok(i) => match i {
if i > count || i == 0 { _i if _i > count || _i == 0 => KeepOrDrop::None,
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-22 19:55:13 +00:00
Outdated
Review
Copy Link

You can make this a bit more concise with yet another match expression:

Ok(i) => match i {
    _i if _i > count || _i == 0 => KeepOrDrop::Keep(count),
    i => KeepOrDrop::Keep(i)
}

This is mostly a stylistic choice though.

You can make this a bit more concise with yet another match expression: ```rust Ok(i) => match i { _i if _i > count || _i == 0 => KeepOrDrop::Keep(count), i => KeepOrDrop::Keep(i) } ``` This is mostly a stylistic choice though.
KeepOrDrop::Keep(count) i => KeepOrDrop::Keep(i),
} else {
KeepOrDrop::Keep(i)
}
}, },
// Err, check if drop works // Err, check if drop works
Err(_) => { Err(_) => {
match drop.parse::<u32>() { match drop.parse::<u32>() {
kg333 marked this conversation as resolved Outdated
projectmoon commented 2021-09-17 06:59:02 +00:00
Outdated
Review
Copy Link

Instead of unwrap, you should make use of the features of the Result type. While you will see examples of Rust code around the internet littered with unwrap, you don't actually want to use it except under a few circumstances:

  • You know the code will not crash.
  • You are okay (or don't care) with the code crashing.

In this case, might be best to combine parsing and validation:

keep.parse().map(|k| {
    //Could also use match block if it's shorter
    if k > count || k == 0 {
        count
    } else {
        k
    }
})?;

This will send up parsing errors back up the call chain and keep the boundary checks in place for a successful parse.

This comment also apples to the drop parsing.

Instead of `unwrap`, you should make use of the features of the Result type. While you will see examples of Rust code around the internet littered with unwrap, you don't actually want to use it except under a few circumstances: - You know the code will not crash. - You are okay (or don't care) with the code crashing. In this case, might be best to combine parsing and validation: ```rust keep.parse().map(|k| { //Could also use match block if it's shorter if k > count || k == 0 { count } else { k } })?; ``` This will send up parsing errors back up the call chain and keep the boundary checks in place for a successful parse. This comment also apples to the `drop` parsing.
kg333 commented 2021-09-18 02:05:31 +00:00
Outdated
Review
Copy Link

This is proving more difficult than I anticipated: it looks like nom is using its own error type? Not sure how to resolve this one, although I agree unwrap is a problem.

error[E0271]: type mismatch resolving `<u32 as std::str::FromStr>::Err == nom::Err<(&str, nom::error::ErrorKind)>`
  --> dicebot/src/basic/parser.rs:56:29
   |
56 |     let count: u32 = (count.parse())?;
   |                             ^^^^^ expected struct `ParseIntError`, found enum `nom::Err`
   |
   = note: expected struct `ParseIntError`
                found enum `nom::Err<(&str, nom::error::ErrorKind)>`
This is proving more difficult than I anticipated: it looks like nom is using its own error type? Not sure how to resolve this one, although I agree unwrap is a problem. ``` error[E0271]: type mismatch resolving `<u32 as std::str::FromStr>::Err == nom::Err<(&str, nom::error::ErrorKind)>` --> dicebot/src/basic/parser.rs:56:29 | 56 | let count: u32 = (count.parse())?; | ^^^^^ expected struct `ParseIntError`, found enum `nom::Err` | = note: expected struct `ParseIntError` found enum `nom::Err<(&str, nom::error::ErrorKind)>` ```
kg333 commented 2021-09-18 03:13:03 +00:00
Outdated
Review
Copy Link

After looking at it further, the weird error type is irrelevant. Every possible arm of the match at this point either assigns Drop or Keep, or leaves a default Keep = Count if the parse fails.

After looking at it further, the weird error type is irrelevant. Every possible arm of the match at this point either assigns Drop or Keep, or leaves a default Keep = Count if the parse fails.
// Ok, there's a drop value, check and create Drop // Ok, there's a drop value, check and create Drop
Ok(i) => { Ok(i) => match i {
if i >= count { _i if i >= count => KeepOrDrop::None,
KeepOrDrop::Keep(count) i => KeepOrDrop::Drop(i),
} else {
KeepOrDrop::Drop(i)
}
}, },
// Err, there's neither keep nor drop // Err, there's neither keep nor drop
Err(_) => KeepOrDrop::Keep(count), Err(_) => KeepOrDrop::None,
} }
}, },
}; };
@ -155,25 +149,25 @@ mod tests {
use super::*; use super::*;
#[test] #[test]
fn dice_test() { fn dice_test() {
assert_eq!(parse_dice("2d4"), Ok(("", Dice::new(2, 4, KeepOrDrop::Keep(2))))); assert_eq!(parse_dice("2d4"), Ok(("", Dice::new(2, 4, KeepOrDrop::None))));
assert_eq!(parse_dice("20d40"), Ok(("", Dice::new(20, 40, KeepOrDrop::Keep(20))))); assert_eq!(parse_dice("20d40"), Ok(("", Dice::new(20, 40, KeepOrDrop::None))));
assert_eq!(parse_dice("8d7"), Ok(("", Dice::new(8, 7, KeepOrDrop::Keep(8))))); assert_eq!(parse_dice("8d7"), Ok(("", Dice::new(8, 7, KeepOrDrop::None))));
assert_eq!(parse_dice("2d20k1"), Ok(("", Dice::new(2, 20, KeepOrDrop::Keep(1))))); assert_eq!(parse_dice("2d20k1"), Ok(("", Dice::new(2, 20, KeepOrDrop::Keep(1)))));
assert_eq!(parse_dice("100d10k90"), Ok(("", Dice::new(100, 10, KeepOrDrop::Keep(90))))); assert_eq!(parse_dice("100d10k90"), Ok(("", Dice::new(100, 10, KeepOrDrop::Keep(90)))));
assert_eq!(parse_dice("11d10k10"), Ok(("", Dice::new(11, 10, KeepOrDrop::Keep(10))))); assert_eq!(parse_dice("11d10k10"), Ok(("", Dice::new(11, 10, KeepOrDrop::Keep(10)))));
assert_eq!(parse_dice("12d10k11"), Ok(("", Dice::new(12, 10, KeepOrDrop::Keep(11))))); assert_eq!(parse_dice("12d10k11"), Ok(("", Dice::new(12, 10, KeepOrDrop::Keep(11)))));
assert_eq!(parse_dice("12d10k13"), Ok(("", Dice::new(12, 10, KeepOrDrop::Keep(12))))); assert_eq!(parse_dice("12d10k13"), Ok(("", Dice::new(12, 10, KeepOrDrop::None))));
assert_eq!(parse_dice("12d10k0"), Ok(("", Dice::new(12, 10, KeepOrDrop::Keep(12))))); assert_eq!(parse_dice("12d10k0"), Ok(("", Dice::new(12, 10, KeepOrDrop::None))));
assert_eq!(parse_dice("20d40dh5"), Ok(("", Dice::new(20, 40, KeepOrDrop::Drop(5))))); assert_eq!(parse_dice("20d40dh5"), Ok(("", Dice::new(20, 40, KeepOrDrop::Drop(5)))));
assert_eq!(parse_dice("8d7dh9"), Ok(("", Dice::new(8, 7, KeepOrDrop::Keep(8))))); assert_eq!(parse_dice("8d7dh9"), Ok(("", Dice::new(8, 7, KeepOrDrop::None))));
assert_eq!(parse_dice("8d7dh8"), Ok(("", Dice::new(8, 7, KeepOrDrop::Keep(8))))); assert_eq!(parse_dice("8d7dh8"), Ok(("", Dice::new(8, 7, KeepOrDrop::None))));
} }
#[test] #[test]
fn element_test() { fn element_test() {
assert_eq!( assert_eq!(
parse_element(" \t\n\r\n 8d7 \n"), parse_element(" \t\n\r\n 8d7 \n"),
Ok((" \n", Element::Dice(Dice::new(8, 7, KeepOrDrop::Keep(8))))) Ok((" \n", Element::Dice(Dice::new(8, 7, KeepOrDrop::None))))
); );
assert_eq!( assert_eq!(
parse_element(" \t\n\r\n 3d20k2 \n"), parse_element(" \t\n\r\n 3d20k2 \n"),
@ -199,7 +193,7 @@ mod tests {
parse_signed_element(" \t\n\r\n- 8d4 \n"), parse_signed_element(" \t\n\r\n- 8d4 \n"),
Ok(( Ok((
" \n", " \n",
SignedElement::Negative(Element::Dice(Dice::new(8, 4, KeepOrDrop::Keep(8)))) SignedElement::Negative(Element::Dice(Dice::new(8, 4, KeepOrDrop::None)))
)) ))
); );
assert_eq!( assert_eq!(
@ -213,7 +207,7 @@ mod tests {
parse_signed_element(" \t\n\r\n+ 8d4 \n"), parse_signed_element(" \t\n\r\n+ 8d4 \n"),
Ok(( Ok((
" \n", " \n",
SignedElement::Positive(Element::Dice(Dice::new(8, 4, KeepOrDrop::Keep(8)))) SignedElement::Positive(Element::Dice(Dice::new(8, 4, KeepOrDrop::None)))
)) ))
); );
} }
@ -225,7 +219,7 @@ mod tests {
Ok(( Ok((
"", "",
ElementExpression(vec![SignedElement::Positive(Element::Dice(Dice::new( ElementExpression(vec![SignedElement::Positive(Element::Dice(Dice::new(
8, 4, KeepOrDrop::Keep(8) 8, 4, KeepOrDrop::None
)))]) )))])
)) ))
); );
@ -244,7 +238,7 @@ mod tests {
Ok(( Ok((
" \n ", " \n ",
ElementExpression(vec![SignedElement::Negative(Element::Dice(Dice::new( ElementExpression(vec![SignedElement::Negative(Element::Dice(Dice::new(
8, 4, KeepOrDrop::Keep(8) 8, 4, KeepOrDrop::None
)))]) )))])
)) ))
); );
@ -257,7 +251,7 @@ mod tests {
SignedElement::Positive(Element::Bonus(7)), SignedElement::Positive(Element::Bonus(7)),
SignedElement::Negative(Element::Bonus(5)), SignedElement::Negative(Element::Bonus(5)),
SignedElement::Negative(Element::Dice(Dice::new(6, 12, KeepOrDrop::Drop(3)))), SignedElement::Negative(Element::Dice(Dice::new(6, 12, KeepOrDrop::Drop(3)))),
SignedElement::Positive(Element::Dice(Dice::new(1, 1, KeepOrDrop::Keep(1)))), SignedElement::Positive(Element::Dice(Dice::new(1, 1, KeepOrDrop::None))),
SignedElement::Positive(Element::Bonus(53)), SignedElement::Positive(Element::Bonus(53)),
]) ])
)) ))

1
dicebot/src/basic/roll.rs
View File

@ -90,6 +90,7 @@ impl Roll for dice::Dice {
match self.keep_drop { match self.keep_drop {
KeepOrDrop::Keep(k) => DiceRoll(rolls,k as usize, 0), KeepOrDrop::Keep(k) => DiceRoll(rolls,k as usize, 0),
KeepOrDrop::Drop(dh) => DiceRoll(rolls,self.count as usize, dh as usize), KeepOrDrop::Drop(dh) => DiceRoll(rolls,self.count as usize, dh as usize),
KeepOrDrop::None => DiceRoll(rolls,self.count as usize, 0),
} }
} }
} }