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.

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

wants to merge 2 commits into from

Conversation

jacksontheel
Copy link

@jacksontheel jacksontheel commented Mar 12, 2022

Closes #240

Adds a functioning conditional (popularly known as ternary) operator. Simple change to code, several tests added to cover this change.

@jacksontheel jacksontheel requested a review from GreenImp as a code owner March 12, 2022 06:07
Copy link
Collaborator

@GreenImp GreenImp left a comment

Choose a reason for hiding this comment

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

Thanks for this! A few small comments below.

If you're not comfortable adding the extra tests, let me know and I can look at getting this merged in to a feature branch and adding the tests.

@@ -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:

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

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:

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

@@ -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

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).

@@ -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

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).

@GreenImp GreenImp force-pushed the develop branch 3 times, most recently from 4ff7fb9 to 03379d6 Compare May 12, 2023 00:16
@jacksontheel jacksontheel closed this by deleting the head repository Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ternary operator
2 participants