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

Bring pipelineOperator flag in line with minimal #2

Conversation

mAAdhaTTah
Copy link
Collaborator

Q                       A
Fixed Issues? N/A
Patch: Bug Fix? Yes
Major: Breaking Change? Yes
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR No
Any Dependency Changes? No
License MIT

The minimal proposal requires parentheses around arrow functions
and bans await from the pipeline. The fsharpPipeline flag will
be responsible for enabling the await-in-pipeline behaviors.

@mAAdhaTTah
Copy link
Collaborator Author

@js-choi For your review.

) {
throw this.raise(
this.state.start,
`Unexpected "await" after pipeline body; await is banned in minimal proposal`,
Copy link

Choose a reason for hiding this comment

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

I think this error message should be tweaked to say that bare pipeline bodies |> await are banned in the minimal proposal, not await in general. The minimal proposal does allow await as long as it’s not |> await. For instance, x |> (await promiseThatWillResolveToFunction). I…think. See tc39/proposal-pipeline-operator#108 (comment).

Copy link
Collaborator Author

@mAAdhaTTah mAAdhaTTah Mar 29, 2018

Choose a reason for hiding this comment

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

Oh, right, I should check that:

x |> (await p)

parses correctly, as that's still allowed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the error message. it parses correctly as-is though.

@js-choi
Copy link

js-choi commented Mar 29, 2018

The changes look good to me, with one exception on which I’ve commented.

The fsharpPipeline flag will be responsible for enabling the await-in-pipeline behaviors.

Have you come to a decision about the F-Sharp Proposal’s precedence between |> and => for now? That’s another potential difference from the Minimal Proposal.

@mAAdhaTTah
Copy link
Collaborator Author

Have you come to a decision about the F-Sharp Proposal’s precedence between |> and => for now? That’s another potential difference from the Minimal Proposal.

I haven't yet, but the current minimal requires it, so I'm punting that decision until I implement it. The parenthesized arrow functions is the only difference between F# and the other two, but F# relies on arrow functions to flesh out its usefulness, so I'm still trying to decide if the trade-off is worth it.

The other alternative would be to hitch F# to partial application and try and advance them both together. Parentheses may not be bad enough to worry about, and it does come with some unpleasant side effects (const a = (x |> f) & const f = x => (x |> a.f)).

The minimal proposal requires parentheses around arrow functions
and bans await from the pipeline. The `fsharpPipeline` flag will
be responsible for enabling the await-in-pipeline behaviors.
@mAAdhaTTah mAAdhaTTah force-pushed the feature/minimal-proposal-compliance branch from 6cc321c to 8bff3f3 Compare March 29, 2018 22:08
@js-choi
Copy link

js-choi commented Mar 29, 2018

Understood. The pull request with 8bff3f3 looks good to me.

@mAAdhaTTah mAAdhaTTah merged commit 21acf48 into feature/pipeline-operator-proposals Mar 29, 2018
@mAAdhaTTah mAAdhaTTah deleted the feature/minimal-proposal-compliance branch March 29, 2018 22:24
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.

2 participants