Preserve function body braces#2223
Conversation
This is an alternative to reasonml#1826 implementing just the brace preservation code, to make it easier to be reviewed. I think this is now in a better place to get into the codebase given that we have `Reason_attributes` outside of `Reason_pprint_ast` and that we now remove our own attributes before printing to OCaml. I think we can probably refactor this code further in future PRs, as well as add other stylistic preservation changes.
anmonteiro
force-pushed
the
anmonteiro/preserve-requested-braces
branch
from
0c5c67c to
c7182e4
Compare
| _ | ||
| )}, | ||
| _ | ||
| ) -> Printf.eprintf "YEP\n%!";true *) |
|
|
||
| let semiTerminated term = makeList [term; atom ";"] | ||
|
|
||
| let makeBreakableBlock ?(inline=false) letItems = |
There was a problem hiding this comment.
I'm not a huge fan of abstractions on top of makeList. The exact behaviour of makeBreakableBlock is very hard to predict without inspecting its implementation.
makeBreakableBlock seems to be used only once too. Is there a reason why you didn't use makeList with all named args in the letList printing? Just using makeList with all named args makes it very clear what is going to happen.
|
yayyyyyyy |
| {partition with literalAttrs=attr::partition.literalAttrs} | ||
| | (({txt="reason.preserve_braces"}, _) as attr) :: atTl -> | ||
| let partition = partitionAttributes ~partDoc ~allowUncurry atTl in | ||
| {partition with literalAttrs=attr::partition.literalAttrs} |
There was a problem hiding this comment.
literalAttrs was originally intended to represent string literals. It seems you're repurposing them here? The name should probably be changed accordingly.
|
Is it currently the case that these (and other stylistic attributes such as string "literal" attributes) are removed when the |
|
@jordwalke yes, we remove I agree with your comment that the name should probably be changed. |
| pub push = hd => { | ||
| v = [hd, ...v]; | ||
| }; | ||
| initializer { |
There was a problem hiding this comment.
What's the rational behind printing braces here, while the original had parens?
There was a problem hiding this comment.
| open M; | ||
| let z = M.(34); | ||
| let z = { | ||
| M.(34); |
There was a problem hiding this comment.
Why does this local open sugar have braces?
There was a problem hiding this comment.
This is how the original input file looked.
There was a problem hiding this comment.
Ah indeed, my bad, was confused between braces and open sugar.
This is an alternative to reasonml#2289 that also renames `literalAttrs` to `stylisticAttrs` as request in reasonml#2223's code review
This is an alternative to reasonml#2289 that also renames `literalAttrs` to `stylisticAttrs` as request in reasonml#2223's code review
6 participants
This is an alternative to #1826 implementing just the brace preservation
code, to make it easier to be reviewed.
I think this is now in a better place to get into the codebase given
that we have
Reason_attributesoutside ofReason_pprint_astand thatwe now remove our own attributes before printing to OCaml.
I think we can probably refactor this code further in future PRs, as
well as add other stylistic preservation changes.