fix: natural 1s/20s now auto fail/succeed #342
Conversation
|
@bakbakbakbakbak in your review summary you wrote "add result.isFailure = true; after row 206 to indiciate a failure" but could you write this where this should go in a review comment annotation instead? I ask because there is no |
I couldn't so I added a commit to the branch when I tried editing the file in github. I don't know if the change makes sense, but since we already have a |
bakbakbakbakbak
dismissed
their stale review
I have reconsidered, we don't need to implement the changes I suggested right now. I think it is a better idea to get the fix out instead of being nit-picky with an unused variable here!
| ) { | ||
| result.details = game.i18n.format( | ||
| "OSE.messages.AttackAscendingFailure", | ||
| { | ||
| bonus: result.target, | ||
| } | ||
| ); | ||
| result.isFailure = true; |
There was a problem hiding this comment.
Perhaps we could consider remove the isFailure value and just check isSuccess which is false by default?
There was a problem hiding this comment.
I've been reconsidering, I think we can disregard this for now!
| @@ -178,6 +181,7 @@ export class OseDice { | |||
| result.details = game.i18n.format("OSE.messages.AttackFailure", { | |||
| bonus: result.target, | |||
| }); | |||
| result.isFailure = true; | |||
There was a problem hiding this comment.
Same as for row 172 but for Ascending AC.
There was a problem hiding this comment.
I've been reconsidering, I think we can disregard this for now!
Fixes #340 and #341
These were accessing the wrong properties of the roll object and not using strong comparison, which is at least part of why it took so long to find a problem here. Thanks to @bakbakbakbakbak's tests for discovering this