Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add conditional/ternary operator #246

Closed
wants to merge 2 commits into from
Closed

Add conditional/ternary operator #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d25f432
Add conditional/ternary operator
Mar 12, 2022
5708bd9
Merge branch 'develop' into feature/240-Conditional-Operator
jacksontheel Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/parser/grammars/grammar.pegjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,13 @@ Number

Operator
= "**" { return "^" } / "*" / "^" / "%" / "/" / "+" / "-"
/ "?" _ expr:Expression _ ":" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would allow ternary expressions without the second / false expression, which would be invalid. eg. ? 10 :

I wonder if it might be worth moving this up to the Expression clause on line 148:

rpg-dice-roller/src/parser/grammars/grammar.pegjs

Lines 148 to 162 in f1e0638

Expression
= head:Factor tail:(_ Operator _ Factor)* {
head = Array.isArray(head) ? head : [head];
return [
...head,
...tail
.map(([, value, , factor]) => {
return [
value,
factor
];
}).flat(2)
]
}

Possibly inside the tail.

That way, we can ensure that it always has a Factor after the :.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. I'll look into implementing it.

Copy link
Author

@jacksontheel jacksontheel Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further investigation my understanding is that if the operator is ? 10 : like in your example, the greater expression would be [factor] ? 10 : [factor]. I might be missing something, but I don't see how missing the 'false' value is possible.

This is indeed restricted by the fact that the 'false' value is a Factor, while the 'true' value may be an Expression. Could this be fixed with a more recursive definition of Expression? Something akin to

Expression
  = head:Factor tail:(_ Operator _ Expression)*

After changing this locally, the existing tests are still passing. I obviously do not know the project like you do, but what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true for that section, where we explicitly have a Factor after the Operator, but it means that we'd need to ensure that wherever Operator is used, we have a Factor afterwards that is suitable to the ternary operator.

Unfortunately, this isn't the case, as Operator is also used for IntegerOrExpression:

rpg-dice-roller/src/parser/grammars/grammar.pegjs

Lines 143 to 145 in f1e0638

IntegerOrExpression
= IntegerNumber
/ l:"(" _ expr:(FloatNumber (_ Operator _ FloatNumber)+) _ r:")" { return evaluate(text()) }

Where ternary operators could cause issues if they include Dice rolls, because the system can't handle things like (d8)d10 or 1d(d10) (Discussed in #206).

Based on the change to Operator, this would be parsed as valid grammar, but will throw an error when run:

(3 < 8 ? 4d8 : 5)d10

I also think the tail part of Expression will now allow things like:

? 10 : 5 ? 3 : 4 ? 20 : 30

Because it allow repetition, and there's no checks to ensure that the part before the ? is actually a true/false expression (Although I'm not too sure we need to care about the latter).

I think it needs to ensure the ternary can't be repeated like that or, if it is, the inner parts are surrounded in parenthesis. e,g:

10 > 5 ? (4 < 6 ? 7 : 10) : 4

After thinking about it for a while, I think it might actually be better to move it into the Factor definition. That way it's available either side of a normal Operator. e.g.:

4 > 5 ? 2 * 3

// technically valid and will output `2`
4 > 5 ? 2 * 3 : 2 * 2 < 2 ? 1 : 2

return [
'?',
...expr,
":",
];
}


_ "whitespace"
Expand Down
14 changes: 14 additions & 0 deletions tests/parser/Parser.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1743,5 +1743,19 @@ describe('Parser', () => {
expect(parsed[2]).toBe(0);
});
});
describe('Conditional Operator', () => {
test('roll `1?5:0', () => {
Copy link
Collaborator

@GreenImp GreenImp Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Could you also add tests to check against ternaries with other value types (ie. Dice, math formulas etc), in condition, and the true / false expressions?
And also some tests to make sure that incorrect syntax fails (e.g. missing either true / false expression).

const parsed = Parser.parse('1?5:0');

expect(parsed).toBeInstanceOf(Array);
expect(parsed).toHaveLength(5);

expect(parsed[0]).toBe(1);
expect(parsed[1]).toEqual('?');
expect(parsed[2]).toBe(5);
expect(parsed[3]).toEqual(':');
expect(parsed[4]).toBe(0);
});
});
});
});
28 changes: 28 additions & 0 deletions tests/rolling.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,34 @@ describe('Rolling', () => {
// remove the spy
spy.mockRestore();
});

test('roll `(d6>3)?1:0', () => {
Copy link
Collaborator

@GreenImp GreenImp Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, looking good. Would you also be able to add tests to ensure that it correctly handles more advanced expressions as well?
And also some tests to make sure that incorrect syntax fails (e.g. missing either true / false expression).

const rolls = new RollResults([4]);
const spy = jest.spyOn(StandardDice.prototype, 'roll')
.mockImplementationOnce(() => rolls);
const roll = roller.roll('(d6>3)?1:0');

expect(roll).toBeInstanceOf(DiceRoll);
expect(roll.notation).toEqual('(d6>3)?1:0');

expect(roll.rolls).toHaveLength(7);
expect(roll.rolls[0]).toEqual('(');
expect(roll.rolls[1]).toEqual(rolls);
expect(roll.rolls[2]).toEqual(')');
expect(roll.rolls[3]).toEqual('?');
expect(roll.rolls[4]).toBe(1);
expect(roll.rolls[5]).toEqual(':');
expect(roll.rolls[6]).toBe(0);

expect(roll.total).toBe(1);
expect(roll.output).toEqual('(d6>3)?1:0: ([4])?1:0 = 1');
expect(roll.minTotal).toEqual(0);
expect(roll.maxTotal).toEqual(1);
expect(roll.averageTotal).toEqual(0.5);

// remove the spy
spy.mockRestore();
});
});

describe('Group rolls', () => {
Expand Down