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

Don't swallow <> </> fragments when formatting #1963

Merged
merged 3 commits into from
May 28, 2018

Conversation

anmonteiro
Copy link
Member

fixes #870

self#formatChildren (remaining @ children) processedRev
| {pexp_desc = Pexp_construct ({txt = Lident "::"}, Some {pexp_desc = Pexp_tuple children} )} as x :: remaining ->
let {jsxAttrs} = partitionAttributes x.pexp_attributes in
if jsxAttrs != [] then
Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is the pattern match on jsxAttrs. Pattern matching is probably cheaper than physical equality checks. (Correct me if I'm wrong).

Copy link
Contributor

@hcarty hcarty May 27, 2018

Choose a reason for hiding this comment

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

Pattern matching should be safer against future changes in compiler optimizations around equality.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, I actually had a pattern match at first but decided to go with this approach given it’s used everywhere else that does the same thing.

@IwanKaramazow
Copy link
Contributor

Can you add some extra tests to verify other expressions?
For example function calls need to be wrapped in braces/parens.

@anmonteiro
Copy link
Member Author

Addressed the comments in the new commits.

@IwanKaramazow
Copy link
Contributor

Great! @chenglou, good from my part

@chenglou
Copy link
Member

Thanks. Will be useful soon I think

@chenglou chenglou merged commit 0502715 into reasonml:master May 28, 2018
@anmonteiro anmonteiro deleted the gh-870 branch May 29, 2018 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatting makes <></> fragment dissapear
5 participants