fix: do not print > on new line #678
Conversation
|
@cknitt could you try on your codebase to check no formatting change? |
src/res_printer.ml
Outdated
| @@ -3978,6 +3978,38 @@ and printPexpApply ~customLayout expr cmtTbl = | |||
|
|
|||
| and printJsxExpression ~customLayout lident args cmtTbl = | |||
| let name = printJsxName lident in | |||
| let hasTailSingleLineComment = | |||
There was a problem hiding this comment.
We seem to traverse the props multiple times. Do we fuse this logic with printJsxProps? There we walk the props one by one. We eventually arrive at the children, so we have all information inside printJsxProps, don't we?
printJsxProps also seems like a good place to try and print the closing >. What do you think of this?
There was a problem hiding this comment.
make sense to me. Will take a look later
There was a problem hiding this comment.
We better not print > in the printJsxProps function, because we have additional logic to determine whether the tag is self-closing, and then choose to print (> and </A>) or />
Yes, will test later today. But as the formatting in the test results is back to what it was before, I think we should be good. |
|
I can confirm that the formatting is back to what it was before. At least as far as JSX is concerned. I had one other formatting change: module PipeFirst = {
}=> module PipeFirst = {}that must have been caused by some other commit. But this will occur rarely in practice (and is definitely an improvement IMHO). |
|
Self-closing tags and normal tags have different formatting behavior. We always put <A
a=""
/>
<A
a="1">
<B />
</A>Should we keep them consistent? |
|
I cannot confirm that. I just tried again with the version I tested yesterday and with 10.0.1 in the playground, and I am getting <A a="" />
<A a="1">
<B />
</A> |
<A
// break tag
a=""
/>
<A
// break tag
a="1">
<B />
</A>Try this? |
|
Ah, right, yes, this does give me <A
// break tag
a=""
/>
<A
// break tag
a="1">
<B />
</A>in the playground. |
|
Should we keep them consistent? |
|
IMHO, in 10.1, we should avoid any format changes. We can make things consistent in 11.0 (always keep the What do you think @cristianoc? |
|
Yes I think avoiding changes is best for 10.1. |
|
@ah-yu is this ready to go, or is there more to do? |
|
Wait a minute, I need more test |
|
I think this is ready to go @cristianoc |
This reverts commit b9f2f7b.
Closes #675