Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

fix: Avoid tail comments inside tag from being eaten #664

Merged
merged 10 commits into from Sep 30, 2022

Conversation

ah-yu
Copy link
Contributor

@ah-yu ah-yu commented Sep 29, 2022

Closes #663

// The minimal reproducible example
<A
a="1"
// comment
>
  <B />
</A>

tweak the output of jsx tag

// before
<A
// comment  
  a="1"
/* comment */> 
  <B />
</A>

// now
<A
  // comment -- align with a="1"
  a="1"
  /* comment */   //  align with a="1"
>       // print > on a separate line -- same as prettier
  <B />
</A>

@ah-yu ah-yu marked this pull request as draft September 29, 2022 11:06
@ah-yu ah-yu marked this pull request as ready for review September 30, 2022 02:18
Copy link
Contributor

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Some questions in the code.

label = Asttypes.Labelled "children")
in
match maybeChildren with
| None -> ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not walking the props at all?
What happens if you have props with comments and no children?

Copy link
Contributor Author

@ah-yu ah-yu Sep 30, 2022

Choose a reason for hiding this comment

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

Would you give me a hint under what circumstances this would happen? I tested some cases and found children were always there

If there are no children elements inside, there will be Pexp_construct "[]" in the AST

Labelled "children"
         expression (test.res[2,1+2]..[2,1+3])
           Pexp_construct "[]" (test.res[2,1+2]..[2,1+3]) ghost
           None

Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a comment saying just that.

| Some (_, children) ->
let leading, inside, _ = partitionByLoc after children.pexp_loc in
if props = [] then
let afterExpr, _ =
Copy link
Contributor

Choose a reason for hiding this comment

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

What happen to the second argument, _ in this case?
Why can it be ignored?

Copy link
Contributor Author

@ah-yu ah-yu Sep 30, 2022

Choose a reason for hiding this comment

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

Because it is an empty list.

<A
// comment
// comment
>
</A>

If there are no props, all comments after A are trailing comments of A

Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a comment saying that.

CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Looks great.
Just asking to add a couple of comments to it's clear the code paths not covered are intentional and not an oversight.

CHANGELOG.md Show resolved Hide resolved
label = Asttypes.Labelled "children")
in
match maybeChildren with
| None -> ()
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a comment saying just that.

| Some (_, children) ->
let leading, inside, _ = partitionByLoc after children.pexp_loc in
if props = [] then
let afterExpr, _ =
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a comment saying that.

@ah-yu
Copy link
Contributor Author

ah-yu commented Sep 30, 2022

@cristianoc Comments added.

@cristianoc cristianoc merged commit 6870d14 into rescript-lang:master Sep 30, 2022
@ah-yu ah-yu deleted the avoid-comments-being-eaten branch September 30, 2022 10:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formatter erasing comments
2 participants