[RFC] Fix comment location for binary expressions #1043

Merged
merged 1 commit into from Mar 21, 2017

Conversation

Projects
None yet
2 participants
@vjeux
Collaborator

vjeux commented Mar 19, 2017

The root cause is that we're calling printComments with an empty string, meaning that the leading/trailing comments are not correctly inserted at the right location.

So, the way to fix it is to call p => concat(parts) but because we're mutating the array in place, it doesn't work. We need to mutate it and create a copy. But, the root call is actually checking the shape of the parts array which our code is now breaking...

          // Don't include the initial expression in the indentation
          // level. The first item is guaranteed to be the first
          // left-most expression.
          parts.length > 0 ? parts[0] : "",

The consequence is that binary expressions are no longer indented correctly if expressions have a comment (but now it places the comment properly!), which seems like an okay trade-off.

I'm not sure if we should merge this one or instead refactor this code such that it doesn't rely on mutation and looking at the shape of the printed tree. printMemberChain is a good thing to reference for inspiration.

Fixes #946

[RFC] Fix comment location for binary expressions
The root cause is that we're calling `printComments` with an empty string, meaning that the leading/trailing comments are not correctly inserted at the right location.

So, the way to fix it is to call `p => concat(parts)` but because we're mutating the array in place, it doesn't work. We need to mutate it and create a copy. But, the root call is actually checking the shape of the parts array which our code is now breaking...

```js
          // Don't include the initial expression in the indentation
          // level. The first item is guaranteed to be the first
          // left-most expression.
          parts.length > 0 ? parts[0] : "",
```

The consequence is that binary expressions are no longer indented if the first expression has a comment (but now it places the comment properly!), which seems like a good trade-off.

I'm not sure if we should merge this one or instead refactor this code such that it doesn't rely on mutation and looking at the shape of the printed tree. `printMemberChain` is a good thing to reference for inspiration.

Fixes #946
+ 0,
+ parts.length,
+ comments.printComments(path, p => concat(parts.slice()), options)
+ );

This comment has been minimized.

@jlongster

jlongster Mar 20, 2017

Member

I think this code could be a little more readable. Doesn't this do the same thing?

parts = comments.printComments(path, p => concat(parts), optons)

?

@jlongster

jlongster Mar 20, 2017

Member

I think this code could be a little more readable. Doesn't this do the same thing?

parts = comments.printComments(path, p => concat(parts), optons)

?

This comment has been minimized.

@vjeux

vjeux Mar 20, 2017

Collaborator

No, parts is an array that's being mutated. You can't just re-assign the variable, you need to clear its contents :(

@vjeux

vjeux Mar 20, 2017

Collaborator

No, parts is an array that's being mutated. You can't just re-assign the variable, you need to clear its contents :(

This comment has been minimized.

@jlongster

jlongster Mar 21, 2017

Member

Where else is the instance being mutated that would create an inconsistency? If you reassign the variable any mutations below will continue to work, operating on the new array.

Sorry for the delay; I'll keep this PR open in a tab and check back in so we can get this merged.

@jlongster

jlongster Mar 21, 2017

Member

Where else is the instance being mutated that would create an inconsistency? If you reassign the variable any mutations below will continue to work, operating on the new array.

Sorry for the delay; I'll keep this PR open in a tab and check back in so we can get this merged.

This comment has been minimized.

@vjeux

vjeux Mar 21, 2017

Collaborator

https://github.com/prettier/prettier/blob/master/src/printer.js#L223

The outer call site creates the part array, passes it as reference and looks at it at the end to do the indentation and to print it.

@vjeux

vjeux Mar 21, 2017

Collaborator

https://github.com/prettier/prettier/blob/master/src/printer.js#L223

The outer call site creates the part array, passes it as reference and looks at it at the end to do the indentation and to print it.

This comment has been minimized.

@vjeux

vjeux Mar 21, 2017

Collaborator

The more I think about it, the more I think we should just refactor this part of the code to avoid doing those mutations.

@vjeux

vjeux Mar 21, 2017

Collaborator

The more I think about it, the more I think we should just refactor this part of the code to avoid doing those mutations.

This comment has been minimized.

@jlongster

jlongster Mar 21, 2017

Member

Ah, yep. I remember now. I think I tried to avoid this mutation at some point but it was pretty hard with the recursive call, and I want to land a working implementation. The recursiveness makes it pretty hard, and the resulting code might be a bit more awkward. I forget the details, but feel free to give it a try.

Worst case we can merge this. It's not the best, but oh well..

@jlongster

jlongster Mar 21, 2017

Member

Ah, yep. I remember now. I think I tried to avoid this mutation at some point but it was pretty hard with the recursive call, and I want to land a working implementation. The recursiveness makes it pretty hard, and the resulting code might be a bit more awkward. I forget the details, but feel free to give it a try.

Worst case we can merge this. It's not the best, but oh well..

This comment has been minimized.

@vjeux

vjeux Mar 21, 2017

Collaborator

Oh yeah, it's not easy to refactor (I would have done it already :P).

If we've got to chose between two evils, I think that this PR is better. It's not going to indent but at least the comments are going to be stable and at the right place (instead of the crazy way they are now).

Also, this is a very very edge case.

@vjeux

vjeux Mar 21, 2017

Collaborator

Oh yeah, it's not easy to refactor (I would have done it already :P).

If we've got to chose between two evils, I think that this PR is better. It's not going to indent but at least the comments are going to be stable and at the right place (instead of the crazy way they are now).

Also, this is a very very edge case.

@jlongster

Ok, let's just get this in

@vjeux vjeux merged commit ce6e897 into prettier:master Mar 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment