Keep/Drop Function #92

kg333 · 2021-09-17T03:36:05Z

kg333 commented

2021-09-17 03:36:05 +00:00

This adds a keep/drop function to basic dice rolls, allowing D&D 5e advantage/disadvantage respectively.

kg333 added 6 commits 2021-09-17 03:36:07 +00:00

2654887d8c Initial commit to add keep to dice struct and preserve parser test cases

1860eaf378 Adding parsing for keeping highest dice

15163ac11d Adding calculations for keep, and adding validation on keep input

dc242182f4 Fix string comparison in keep/count check, and add test cases

069ee47364 Adding drop function

continuous-integration/drone/pr Build is passing Details

8317f40f61 Updating README for keep/drop

projectmoon reviewed 2021-09-17 07:05:08 +00:00

projectmoon left a comment

Thank you for your pull request! I have made some comments. The most important one at the moment is getting rid of the unwrap() calls so that errors are propagated up the chain instead of potentially causing a panic (though not sure how possible that actually is). The remainder are currently stylistic.

Some questions we need to think about:

Is it possible to have both a keep and a drop in the same roll expression? If so, what does that mean? If not, it's probably better to convert the keep and drop variables on the struct into an enum so that only can be present.
I'm not a huge fan of using d to represent the drop, mostly because the d is also used for the die roll itself. But I also can't think of a better letter/word to use for indicating the drop.

Thank you for your pull request! I have made some comments. The most important one at the moment is getting rid of the `unwrap()` calls so that errors are propagated up the chain instead of potentially causing a panic (though not sure how possible that actually is). The remainder are currently stylistic. Some questions we need to think about: - Is it possible to have both a keep and a drop in the same roll expression? If so, what does that mean? If not, it's probably better to convert the `keep` and `drop` variables on the struct into an enum so that only can be present. - I'm not a huge fan of using `d` to represent the drop, mostly because the `d` is also used for the die roll itself. But I also can't think of a better letter/word to use for indicating the drop.

dicebot/src/basic/parser.rs Outdated

					
				@ -37,0 +36,4 @@

				    // parse main dice expression

				    let (mut input, (count, _, sides)) = tuple((digit1, tag("d"), digit1))(input)?;

				    // check for keep expression (i.e. D&D 5E Advantage)

projectmoon commented

2021-09-17 06:51:15 +00:00

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.

kg333 marked this conversation as resolved

dicebot/src/basic/parser.rs Outdated

					
				@ -37,0 +63,4 @@

				    let count: u32 = count.parse().unwrap();

				    // don't allow keep greater than number of dice, and don't allow keep zero

				    let mut keep: u32  = keep.parse().unwrap();

projectmoon commented

2021-09-17 06:59:02 +00:00

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

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

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.

kg333 marked this conversation as resolved

dicebot/src/basic/roll.rs Outdated

					
				@ -20,3 +20,3 @@

				#[derive(Debug, PartialEq, Eq, Clone)]

				pub struct DiceRoll(pub Vec<u32>);

				// array of rolls in order, how many dice to keep, and how many to drop

projectmoon commented

2021-09-17 07:01:48 +00:00

Add a third / to get the Rustdoc working. It may also be good to more thoroughly describe what keep and drop mean here, namely that they mean keeping the highest or dropping the highest rolls.

Add a third `/` to get the Rustdoc working. It may also be good to more thoroughly describe what keep and drop mean here, namely that they mean keeping the highest or dropping the highest rolls.

kg333 marked this conversation as resolved

kg333 commented

2021-09-18 01:00:24 +00:00

I'll give those changes a stab. While in theory you could have both "drop highest" and "keep highest i.e. drop lowest" on the same roll, I didn't provide a way to enter it, so an enum make sense here.

Regarding 'd' for drop, I was intending to match Roll20, but on second glance, it uses 'd' for dropping lowest dice, not highest dice. Roll20 instead uses "dh" for the function I'm trying to implement here.

Alternatively, Avrae uses 'p' to indicate dropped dice, with "ph" indicating dropped highest. It also has a native advantage/disadvantage call, but that works somewhat differently.

Thinking a change to "dh" for now, thoughts?

EDIT: Leaving it for tonight, but still need to do the enum conversion.

I'll give those changes a stab. While in theory you could have both "drop highest" and "keep highest i.e. drop lowest" on the same roll, I didn't provide a way to enter it, so an enum make sense here. Regarding 'd' for drop, I was intending to match Roll20, but on second glance, it uses 'd' for dropping *lowest* dice, not highest dice. Roll20 instead uses "dh" for the function I'm trying to implement here. Alternatively, Avrae uses 'p' to indicate dropped dice, with "ph" indicating dropped highest. It also has a native advantage/disadvantage call, but that works somewhat differently. Thinking a change to "dh" for now, thoughts? EDIT: Leaving it for tonight, but still need to do the enum conversion.

kg333 added 1 commit 2021-09-18 02:07:38 +00:00

continuous-integration/drone/pr Build is failing Details

f904e3a948 Updating match blocks for keep/drop

kg333 added 1 commit 2021-09-18 02:08:57 +00:00

continuous-integration/drone/pr Build is failing Details

1992ef4e08 Updating roll doc

kg333 added 1 commit 2021-09-18 02:18:31 +00:00

continuous-integration/drone/pr Build is passing Details

8b5973475f Forgot to fix tests, fixing keep/drop Err case

kg333 added 1 commit 2021-09-18 03:11:20 +00:00

continuous-integration/drone/pr Build is passing Details

3d6210b32d Adding enum for exclusive drop/keep

kg333 added 1 commit 2021-09-18 03:16:02 +00:00

continuous-integration/drone/pr Build is passing Details

2d9853fbf0 Updating README for new drop command

kg333 commented

2021-09-18 03:40:36 +00:00

Enum conversion is complete for dice.rs. I left the separate usizes on DiceRoll in place though - I couldn't see a way to update it without complicating DiceRoll::total unnecessarily.

Bot has been rebuilt and function look OK.

Enum conversion is complete for dice.rs. I left the separate usizes on DiceRoll in place though - I couldn't see a way to update it without complicating DiceRoll::total unnecessarily. Bot has been rebuilt and function look OK.

projectmoon reviewed 2021-09-22 19:51:54 +00:00

dicebot/src/basic/dice.rs Outdated

					
				@ -18,2 +19,3 @@

				    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {

				        write!(f, "{}d{}", self.count, self.sides)

				        match self.keep_drop {

				            KeepOrDrop::Keep(keep) => {

projectmoon commented

2021-09-22 19:51:54 +00:00

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

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

kg333 marked this conversation as resolved

projectmoon reviewed 2021-09-22 19:53:33 +00:00

dicebot/src/basic/parser.rs Outdated

					
				@ -37,0 +39,4 @@

				    // check for keep expression to keep highest dice (2d20k1)

				    let (keep, input) = match tuple::<&str, _, (_, _), _>((tag("k"), digit1))(input) {

				        // if ok, keep expression is present

				        Ok(r) => (r.1.1, r.0),

projectmoon commented

2021-09-22 19:53:33 +00:00

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

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

kg333 marked this conversation as resolved

projectmoon reviewed 2021-09-22 19:55:13 +00:00

dicebot/src/basic/parser.rs Outdated

					
				@ -37,0 +58,4 @@

				    let keep_drop = match keep.parse::<u32>() {

				        // Ok, there's a keep value, check and create Keep

				        Ok(i) => {

				            if i > count || i == 0 {

projectmoon commented

2021-09-22 19:55:13 +00:00

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.

kg333 marked this conversation as resolved

kg333 added 1 commit 2021-09-25 03:54:07 +00:00