dice: invalid dice rolls now gives a more friendly error #1386
Conversation
|
Catching specific exceptions: Yay! But in some cases Aside from nitpicking which exception types should be caught, there's also the matter of error wording. The phrase "a weird dice" is grammatically incorrect ("dice" is plural), and "How do I throw it?" would be wrong unless the user gives only one die (the singular form of "dice"). What about one of these alternatives, as a start? Just to avoid Sopel referencing the number of dice.
Something along those lines. I'm not necessarily perfectly happy with any of those, but I would accept one of them if we can't come up with something better. |
|
Yeah, you're right with the grammar. I'll have to think of something more clever.
Since there is no way to send types other than |
|
Catching
So, while in Python 3.5+ those conditions raise |
|
Yeah, you're right about that. I added it to the exception catching, but since IRC does not allow NUL in strings, this should still not happen (See RFC-1459). EDIT: Why the assertion error in |
|
The test failure's not your fault. Looks like one of the DuckDuckGo examples changed in the last several hours. I'll fix it and rerun the tests before merging to get a check-mark on this PR when it's ready. (Or maybe it will fix itself, who knows. The search module tests are annoyingly flaky.) I think I like the new error messages. Just don't quite understand where "as well as the algorithms" comes from for It's probably worth handling |
|
What I mean with "as well as the algorithms" is that |
| bot.reply("You roll %s: %s = very big" % ( | ||
| trigger.group(2), pretty_str)) | ||
| return | ||
| except (SyntaxError, eval_equation.Error): |
There was a problem hiding this comment.
This reference to sopel.tools.calculation.ExpressionEvaluator.Error is weird done this way, but it's not really possible to do it more conventionally due to the nested classes. Trust me, I tried! 😹
Frustratingly, doing e.g. from sopel.tools.calculation.ExpressionEvaluator import Error as ExpressionEvaluationError doesn't work. Nor does from sopel.tools.calculation import ExpressionEvaluator.Error as ExpressionEvaluationError. That's invalid syntax.
I'm not up for refactoring sopel.tools.calculation any time soon, just to "fix" this one except statement to look more conventional, so the module and this PR will do as-is. Maybe a future Sopel release (major version) will eventually make it so the calculation tools actually provides a proper ExpressionEvaluationError that can be imported on its own for catching exceptions… but that's definitely not happening in the point release that will include this PR.
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.
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.
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.
Another side note is that i used
SyntaxErrorrather than the all-encompassingException, as some exceptions should be handled different. As far as I know, though,eval_equation()only throwsSyntaxError.Fixes #1189