Skip to content
This repository has been archived by the owner on Jan 28, 2023. It is now read-only.

Give nullish coalescing lower precedence than logical OR. #40

Conversation

DanielRosenwasser
Copy link
Member

This allows us to have a PR ready to go if we decide that we just want ?? to have lower precedence than || (rather than disallowing ?? from mixing with || as in #38 and #39).

@DanielRosenwasser
Copy link
Member Author

spec.html Outdated Show resolved Hide resolved
Copy link
Member

@rkirsling rkirsling left a comment

Choose a reason for hiding this comment

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

Seems like everywhere in the spec that LogicalORExpression occurs requires an update, so it would be good to include them here, even if they're all just routine additions.

(Actually some of them should already be there, even if we don't don't add NullishExpression. 🤔)

@DanielRosenwasser
Copy link
Member Author

Whew, okay, it's been updated. Similarly, #39 is also updated.

spec.html Outdated
NullishExpression[In, Yield, Await] :
LogicalORExpression[?In, ?Yield, ?Await]
NullishExpression[?In, ?Yield, ?Await] `??` LogicalORExpression[?In, ?Yield, ?Await]
</ins>
</emu-grammar>
</emu-clause>
Copy link
Member

Choose a reason for hiding this comment

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

Seems like these two tentative <emu-clause>s can be deleted if they're now incorporated into the context below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now both here and in #39.

@DanielRosenwasser
Copy link
Member Author

@rkirsling @codehag @zenparsing, if you get the chance to review this after #42, that would be really great ❤️

Copy link
Member

@zenparsing zenparsing left a comment

Choose a reason for hiding this comment

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

Nice and simple.

@rkirsling rkirsling mentioned this pull request Jul 18, 2019
@codehag
Copy link

codehag commented Jul 19, 2019

This is a really nice change. I am also rooting for this one.

spec.html Outdated Show resolved Hide resolved
@jridgewell
Copy link
Member

I think this was voted down at committee.

@rkirsling
Copy link
Member

rkirsling commented Jul 24, 2019

Oh well. My fingers are still crossed for reraising this as a future needs-consensus PR. 😁🤞

@DanielRosenwasser
Copy link
Member Author

I hope I was able to present the argument for this change in a reasonable way. This didn't make it, but ultimately I'm glad that we were able to reach consensus for stage 3! I think the language really is no worse off, and at the very least, we are in a position to reevaluate at a later time if we believe it is. Thank you all very much for reviewing this change and making it possible to present both views.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants