-
Notifications
You must be signed in to change notification settings - Fork 428
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
Uniform parenthesized syntax for abstraction and application #1299
Conversation
GC stat after ============== parsing: allocated_words: 37657442 minor_words: 15582165 promoted_words: 2166069 major_words: 24241346 minor_collections: 103 major_collections: 24 heap_words: 7015424 heap_chunks: 20 top_heap_words: 7015424 compactions: 0 reformatting: allocated_words: 153497667 minor_words: 131135436 promoted_words: 36777601 major_words: 59139832 minor_collections: 553 major_collections: 41 heap_words: 9278464 heap_chunks: 22 top_heap_words: 9278464 compactions: 0 GC stat before ============== parsing: allocated_words: 41243812 minor_words: 19168535 promoted_words: 3911746 major_words: 25987023 minor_collections: 117 major_collections: 25 heap_words: 7015424 heap_chunks: 20 top_heap_words: 7015424 compactions: 0 reformatting: allocated_words: 159464070 minor_words: 137101839 promoted_words: 39600932 major_words: 61963163 minor_collections: 576 major_collections: 43 heap_words: 8068096 heap_chunks: 21 top_heap_words: 8068096 compactions: 0
GC stat after ============= parsing: allocated_words: 35870858 minor_words: 13795581 promoted_words: 2141052 major_words: 24216329 minor_collections: 96 major_collections: 24 heap_words: 7015424 heap_chunks: 20 top_heap_words: 7015424 compactions: 0 reformatting: allocated_words: 149597673 minor_words: 127235442 promoted_words: 36784443 major_words: 59146674 minor_collections: 539 major_collections: 42 heap_words: 395776 heap_chunks: 1 top_heap_words: 9278464 compactions: 2
Replace a global buffer by a buffer local to parsing code. This fixes a bug when processing multiple inputs. For instance with refmt a.re b.re, b.re comments will (incorrectly) be read from a buffer that begins with content from a.
Regarding partial application - we already have warnings/errors for partial application in sequences ( EDIT |
@let-def I'm getting the following:
Freshly updated OPAM, OCaml 4.03.0 Edit: works with 4.04 |
Putting this here in case we forget:
|
Ahah yes, though for now I chose to allow |
Ok, I implemented
Right now the second interpretation is chosen, which means that a list in a JSX list has to be wrapped between parentheses. I am not familiar enough with JSX to know what is more appropriate. |
Latter is fine for now; I don't think I've seen this pattern very often |
@@ -399,7 +399,7 @@ module rec A: { | |||
| Leaf(string) | |||
| Node(ASet.t); | |||
let compare (t1, t2) = | |||
switch ((t1, t2)) { | |||
switch (t1, t2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@let-def this is great.
So it was already valid syntax for the parser, but the pretty-printer was not using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly.
Could |
@let-def this is still weird:
The array should be on the inside. |
I'm concerned about the overloading of colon for types and named arguments. I think it will make things confusing coming from typescript/flow. Since named arguments are foreign to JS, would it make sense to keep the OCaml syntax and use ~ to denote a named arg? |
Ok folks, time to merge this. We've cut a release before this PR that includes the pipe formatting PR (a sore refmt bug before, thanks again @IwanKaramazow). It's now live on OPAM. Don't forget, 1.13.7, not 2.0.0. We've decided on a final release before landing this PR because we didn't want it to look like we're conning you into using this syntax just for the pipe formatting =P. If you were/are against this function syntax change, hopefully you can see that we try our best to make sure folks are happy with most of our changes! And thanks for the discussion so far. We've let it drag on a year because we understand this is a big topic. If we've ignored any opinion, it's accidental. Additionally, as @let-def pointed out, the change does have some essence beyond pure bikeshedding. If the syntax ends up unpopular, then we can always revisit this decision. But give us some time to iterate on it. Speaking of iteration: we will be giving you a smooth upgrade path when this is ready. The PR's the first step; the outcome printer, some tests and the error messages need to be fixed. After that, we'll consider changing some others small things, like @rickyvetter's suggestion. I'll then go test on our internal codebase and let my teammates suffer a bit =). Finally, we'll release the upgrade util if things go well. Thanks again for all the input! And remember: most of you here are Reason "veteran" now. Whether you like this change or not, this PR isn't for you; it's for the future folks you'll be helping. |
This PR pushes #1236 a deeper.
It is not yet ready for merging but some feedback could help further development
Expressions
Arrow type
Type constructors
Value constructors
Modules
Bindings
Previously,
=
was used for binding most values but the sugar for binding functions used=>
.Now
=
is always allowed and{ ... }
can be used for function bodies.Classes