dice: bugfixes, type hints, and additional tests #2532
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dgw
added
Tests
Bugfix
SnoopJ
approved these changes
Oct 27, 2023
Exirel
requested changes
Oct 28, 2023
dgw
commented
Oct 28, 2023
SnoopJ
approved these changes
Oct 28, 2023
Stops the bot replying with an error *and also* outputting a result. Only one or the other should be possible. Error cases should force the user to fix their dice expression to get an accurate roll.
A selection of rolled dice can pass the equation evaluator's complexity checks but still yield a result that's too large for `str` conversion. https://docs.python.org/3.12/library/stdtypes.html#int-max-str-digits
The equation evaluator has been taking care of this since 2013, when the `addition` argument was hard-coded to `0` in a dice-rolling rewrite. See commit 5a8ecf4 for some of the history. A decade later, in 2023, we have no use for this argument. The code to handle it is unreachable, which affects the plugin's coverage score. Best solution is to just remove the dead code.
There are still 3 missed lines (2 partial branches) that I don't believe are possible to test using the example decorator. Both missed branches check for invalid input: one for a non-string expression hypothetically passed to `eval_equation()`, and the other for a non-dice-like pattern hypothetically passed to `_roll_dice()`. So much input manipulation is already happening upstream of these sanity checks that it's *probably* impossible to reach them. For now, I'll be satisfied with 98% coverage in this file. In a later development cycle (i.e. not for 8.0), maybe we should convert this set of tests to a regular test file in `test/modules/test_modules_calc.py` and leave the examples for user documentation only. That would let us have actual *unit* tests for the various components, and cover the probably-unreachable-but-makes-us-feel-safe sanity checks too.
Not that these docstrings are visible anywhere, but it still feels good. One particularly tricky thing with mypy is because `_roll_dice()` is allowed to return `None`, so when building a list of dice rolls the type of the resulting variable MUST be `list[Optional[DicePouch]]`. This is a problem later, where the values of that list are passed to private helper functions that only accept `DicePouch`, not `Optional[DicePouch]` (i.e. they don't handle `None`). My solution was to save dice-roll results as `list[Optional[DicePouch]]` and check if any of them returned `None` to indicate a problem (aborting if so), then filter the results into a new `list[DicePouch]` variable for further processing. I don't like it that much, but it was the lowest impact change I saw available. The "more correct" solution is probably refactoring to make `DicePouch` raise an exception on error, instead of returning `None`, but that seemed out of scope for an annotations patch.
Raise an exception if something goes wrong. Let the caller take care of sending output to IRC based on the error.
We still have uncovered lines/missed branches, and this rewrite actually takes the file coverage from 97% to 96%, but the same solution remains: Real unit tests. The example decorator just can't reach all edge cases.
Python 3.5 changed `compile()` (which `eval_equation()` uses, far down the call stack) to raise `ValueError` in the case that used to raise a `TypeError`, so that block isn't necessary any more. See https://docs.python.org/3.8/library/functions.html#compile and #1386 (comment) Input that doesn't contain any dice expressions could still be processed and wind up at the wrong error handler; added an early-return for that. (An expression without dice rolls should be evaluated using `.calc`. And yes, I'm aware that this could be someone's spacebar heater. Tough!) The tests also now specifically cover an expression with no number of dice (to verify that the default of 1 die works), and cases where the input contains one or more valid dice expression(s) but has invalid syntax somewhere else in the string.
Matching on the dice expression pattern only once is more efficient, and it simplifies the code. Passing a dice expression `re.Match` to `_roll_dice()` eliminates any need for the pesky `InvalidDiceExpressionError` case that was impossible to cover using only the example decorator (and the `if result is None` check was a recent addition to help the type-checker, rather than an actual runtime requirement). I only wish that there was an equivalent to `re.findall()` that returns a *list* of `re.Match` objects instead of an iterator, but the list comprehension required to use `re.finditer()` isn't *that* bad.
As Exirel pointed out, 0 isn't a negative number, and who knows? The plugin's behavior could start to allow 0-sided dice in the future.
Exirel
approved these changes
Oct 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What it says on the tin, really; but the highlights (other than added test cases) are:
DicePouchleft over from a refactor in ~2013 (more details in c35c246's commit message).eval_equationraisesValueError(really big) and when the result is small enough to calculate but still too big for Python'sint->strconversion._roll_dice()helper.Follow-up to #2503 (comment).
Checklist
make qa(runsmake lintandmake test)Notes
Sadly, I believe 100% test coverage is impossible using
@plugin.examplealone, without further modifying the plugin's logic. I gotsopel/modules/dice.pyup to 97%, which is still pretty good. A more in-depth explanation of where I ran into trouble is in the commit message for 7ab4be1.This is easily bumped to 8.0.1 if it takes too long to get settled, but I would rather change the constructor interface of
DicePouchin 8.0.0 even though thesopel.modules.*subpackages aren't really part of the public API. The bugfixes are nice, too.