Add newline for bracket-less arrow functions that return calls #927

Merged
merged 1 commit into from Apr 11, 2017

Conversation

Projects
None yet
7 participants
@vjeux
Collaborator

vjeux commented Mar 7, 2017

I've tried a lot of places where to fix this and this is the only one that gives correct results. This is likely not exhaustive but works for that use case. It's been reported twice in issues and I've seen it happen a few other times so we probably want to get it fixed.

Fixes #922
Fixes #797
Fixes #974

@cpojer

This comment has been minimized.

Show comment
Hide comment

cpojer commented Mar 7, 2017

❤️

@kimjoar

This comment has been minimized.

Show comment
Hide comment
@kimjoar

kimjoar Mar 10, 2017

Contributor

I ❤️ this change!

Contributor

kimjoar commented Mar 10, 2017

I ❤️ this change!

error =>
next(
actionWith({
type: failureType,
error: error.message || \\"Something bad happened\\"
})
)
+

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

All of the other examples are better but this one output is very weird.

@vjeux

vjeux Mar 16, 2017

Collaborator

All of the other examples are better but this one output is very weird.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 16, 2017

Collaborator

Okay, I'm now confident about this fix. Before I was checking if the parent and body were call expressions but this just happened to be a special case. The problem happens when the arrow function is expanded as last argument and we are adding a level of indentation.

The fix is just to add the softline when the arrow function is expanded as last argument.

Collaborator

vjeux commented Mar 16, 2017

Okay, I'm now confident about this fix. Before I was checking if the parent and body were call expressions but this just happened to be a special case. The problem happens when the arrow function is expanded as last argument and we are adding a level of indentation.

The fix is just to add the softline when the arrow function is expanded as last argument.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 19, 2017

Collaborator

@difelice great catch! I can reproduce, looking into a solution now.

For the docs that do not build, I fixed it in #1024, sorry about that.

Collaborator

vjeux commented Mar 19, 2017

@difelice great catch! I can reproduce, looking into a solution now.

For the docs that do not build, I fixed it in #1024, sorry about that.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 19, 2017

Collaborator

I just pushed a fix for it, here is the snapshot that changed:

image

I forgot to add ifBreak and "all" option /facepalm

Collaborator

vjeux commented Mar 19, 2017

I just pushed a fix for it, here is the snapshot that changed:

image

I forgot to add ifBreak and "all" option /facepalm

@difelice

This comment has been minimized.

Show comment
Hide comment
@difelice

difelice Mar 19, 2017

Contributor

Thank-you!

Contributor

difelice commented Mar 19, 2017

Thank-you!

@difelice

This comment has been minimized.

Show comment
Hide comment
@difelice

difelice Mar 19, 2017

Contributor

@vjeux I found another issue:

Add this case in arrow_call.js:

func(
  veryLoooooooooooooooooooooooongName,
  veryLooooooooooooooooooooooooongName =>
    veryLoooooooooooooooongName.something()
);
- Snapshot
+ Received

func(
  veryLoooooooooooooooooooooooongName,
  veryLooooooooooooooooooooooooongName =>
    veryLoooooooooooooooongName.something()
+  
);

- Snapshot
+ Received

func(
  veryLoooooooooooooooooooooooongName,
  veryLooooooooooooooooooooooooongName =>
    veryLoooooooooooooooongName.something(),
+  ,
);

I hope this helps to reproduce.

I just read commands.md, I'll try to give a hand.

Thanks.

Update

Test tweaking printWidth:
image

Update 2

Replace huge retina screenshots with formatted code. Sorry about that.

Contributor

difelice commented Mar 19, 2017

@vjeux I found another issue:

Add this case in arrow_call.js:

func(
  veryLoooooooooooooooooooooooongName,
  veryLooooooooooooooooooooooooongName =>
    veryLoooooooooooooooongName.something()
);
- Snapshot
+ Received

func(
  veryLoooooooooooooooooooooooongName,
  veryLooooooooooooooooooooooooongName =>
    veryLoooooooooooooooongName.something()
+  
);

- Snapshot
+ Received

func(
  veryLoooooooooooooooooooooooongName,
  veryLooooooooooooooooooooooooongName =>
    veryLoooooooooooooooongName.something(),
+  ,
);

I hope this helps to reproduce.

I just read commands.md, I'll try to give a hand.

Thanks.

Update

Test tweaking printWidth:
image

Update 2

Replace huge retina screenshots with formatted code. Sorry about that.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 19, 2017

Collaborator

Excellent, thanks for digging through it. So it double prints when the outer breaks. That's going to be interesting to fix!

Collaborator

vjeux commented Mar 19, 2017

Excellent, thanks for digging through it. So it double prints when the outer breaks. That's going to be interesting to fix!

@vjeux vjeux changed the title from Add newline for bracket-less arrow functions that return calls to [WIP] Add newline for bracket-less arrow functions that return calls Mar 20, 2017

@real34

This comment has been minimized.

Show comment
Hide comment
@real34

real34 Apr 2, 2017

Hi! Thanks for this work, along with #1066.

If it can help to gain confidence with these improvements (and maybe add a few edge cases) I created a compilation of related issues I found while prettifying our project.

Please let me know if you need more details on this

real34 commented Apr 2, 2017

Hi! Thanks for this work, along with #1066.

If it can help to gain confidence with these improvements (and maybe add a few edge cases) I created a compilation of related issues I found while prettifying our project.

Please let me know if you need more details on this

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 4, 2017

Collaborator

@real34 this is an awesome list, it's definitely going in as a test!

Collaborator

vjeux commented Apr 4, 2017

@real34 this is an awesome list, it's definitely going in as a test!

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 5, 2017

Collaborator

Just pushed an update which fixes all the tests. Thanks a lot for this list!

This is how it now looks: https://gist.github.com/vjeux/83488a218252b4030daf6b0d01527fb6

Collaborator

vjeux commented Apr 5, 2017

Just pushed an update which fixes all the tests. Thanks a lot for this list!

This is how it now looks: https://gist.github.com/vjeux/83488a218252b4030daf6b0d01527fb6

@vjeux vjeux changed the title from [WIP] Add newline for bracket-less arrow functions that return calls to Add newline for bracket-less arrow functions that return calls Apr 5, 2017

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Apr 6, 2017

Contributor

Amazing, can't wait for this! This is the only thing prettier has done that reduced readability for me, maybe except for #1099. Good job!

Contributor

SimenB commented Apr 6, 2017

Amazing, can't wait for this! This is the only thing prettier has done that reduced readability for me, maybe except for #1099. Good job!

@real34

This comment has been minimized.

Show comment
Hide comment
@real34

real34 Apr 6, 2017

I agree with @SimenB, the result is awesome! Thank you for your time on this.

The only one I find weird in the test is: https://gist.github.com/vjeux/83488a218252b4030daf6b0d01527fb6#file-test-js-L22

const loadPricesBatch = skus => {
    const params = {};
-   return Observable.fromPromise(
-     axiosInstance.get("/frontcommerce/price", { params }),
-   )
+   return Observable
+     .fromPromise(
+       axiosInstance.get("/frontcommerce/price", { params }),
+     )
      .map(response => response.data)
      .map(reorderForSkus(skus));
};

... but it is a matter of taste I guess :)

real34 commented Apr 6, 2017

I agree with @SimenB, the result is awesome! Thank you for your time on this.

The only one I find weird in the test is: https://gist.github.com/vjeux/83488a218252b4030daf6b0d01527fb6#file-test-js-L22

const loadPricesBatch = skus => {
    const params = {};
-   return Observable.fromPromise(
-     axiosInstance.get("/frontcommerce/price", { params }),
-   )
+   return Observable
+     .fromPromise(
+       axiosInstance.get("/frontcommerce/price", { params }),
+     )
      .map(response => response.data)
      .map(reorderForSkus(skus));
};

... but it is a matter of taste I guess :)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 6, 2017

Collaborator

For fromPromise, if the call fit in a single line it would have stayed on the same line as Observable. It's a bit weird here but there were other cases where it completely broke so I figured it would be a good tradeoff

Collaborator

vjeux commented Apr 6, 2017

For fromPromise, if the call fit in a single line it would have stayed on the same line as Observable. It's a bit weird here but there were other cases where it completely broke so I figured it would be a good tradeoff

+ } else {
+ // We want to print the last argument with a special flag
+ let i = 0;
+ path.each(function(argPath) {

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

What is this path.each for? argPath is never even used

@jlongster

jlongster Apr 11, 2017

Member

What is this path.each for? argPath is never even used

This comment has been minimized.

@vjeux

vjeux Apr 11, 2017

Collaborator

path.call below should instead be argPath.call. It happens to be working because path is being mutated during the traversal. Here I want to iterate on all the arguments and reprint the last one with the special flag.

@vjeux

vjeux Apr 11, 2017

Collaborator

path.call below should instead be argPath.call. It happens to be working because path is being mutated during the traversal. Here I want to iterate on all the arguments and reprint the last one with the special flag.

src/printer.js
+ printedLastArgExpanded = printed
+ .slice(0, -1)
+ .concat(path.call(
+ print.withArgs({expandLastArg: true})

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

I don't understand why you need withArgs. Why not do path.call(p => print(p, { expandLastArg: true })))? Am I missing what's wrong there?

@jlongster

jlongster Apr 11, 2017

Member

I don't understand why you need withArgs. Why not do path.call(p => print(p, { expandLastArg: true })))? Am I missing what's wrong there?

This comment has been minimized.

@vjeux

vjeux Apr 11, 2017

Collaborator

The second argument options are the printer options (printWidth, useTabs...) and not calling the function with specific arguments.

I thought about doing {...options, expandLastArg: true} but the issue is that those options are forwarded down recursively. So this would mean that I'd also have to make sure to unset the expandLastArg before forwarding down otherwise the entire sub-tree would have this flag set, instead of just the node.

@vjeux

vjeux Apr 11, 2017

Collaborator

The second argument options are the printer options (printWidth, useTabs...) and not calling the function with specific arguments.

I thought about doing {...options, expandLastArg: true} but the issue is that those options are forwarded down recursively. So this would mean that I'd also have to make sure to unset the expandLastArg before forwarding down otherwise the entire sub-tree would have this flag set, instead of just the node.

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

print is not genericPrint. This is pretty confusing, and we should probably make this clearer somehow. print is the function passed in, which is printGenerically, which you adds the args argument below. The function in turn calls genericPrint with all this arguments: p, options, printGenerically, args. So the options that I passed in my snippet is the args that is being passed as the fourth argument to genericPrint.

I think that's right?

@jlongster

jlongster Apr 11, 2017

Member

print is not genericPrint. This is pretty confusing, and we should probably make this clearer somehow. print is the function passed in, which is printGenerically, which you adds the args argument below. The function in turn calls genericPrint with all this arguments: p, options, printGenerically, args. So the options that I passed in my snippet is the args that is being passed as the fourth argument to genericPrint.

I think that's right?

This comment has been minimized.

@vjeux

vjeux Apr 11, 2017

Collaborator

It works, you are totally right! Just updated the PR :)

@vjeux

vjeux Apr 11, 2017

Collaborator

It works, you are totally right! Just updated the PR :)

This comment has been minimized.

@jlongster

jlongster Apr 11, 2017

Member

Cool! Yeah, how those functions work are super confusing, and we probably don't need it to be that way (it was from recast which did a lot of other things where this was more helpful)

@jlongster

jlongster Apr 11, 2017

Member

Cool! Yeah, how those functions work are super confusing, and we probably don't need it to be that way (it was from recast which did a lot of other things where this was more helpful)

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

Two small questions, but this looks good in general!

Member

jlongster commented Apr 11, 2017

Two small questions, but this looks good in general!

Add newline for bracket-less arrow functions that return calls
I've tried a lot of places where to fix this and this is the only one that gives correct results. This is likely not exhaustive but works for that use case. It's been reported twice in issues and I've seen it happen a few other times so we probably want to get it fixed.

Fixes #922
Fixes #797
@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

Awesome, looks good to me!

Member

jlongster commented Apr 11, 2017

Awesome, looks good to me!

@vjeux vjeux merged commit e0eb438 into prettier:master Apr 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

vjeux added a commit to vjeux/prettier that referenced this pull request Apr 12, 2017

Fix edge cases triggered by newlines in arrow functions
This one is pretty crazy. In #927, I changed

```js
concat(["(", join(concat([", "]), printed), ")"]),
```

into

```js
concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),
```

which makes the example in #1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.

In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.

The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.

The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.

I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

Fix #1203

vjeux added a commit to vjeux/prettier that referenced this pull request Apr 12, 2017

Fix edge cases triggered by newlines in arrow functions
This one is pretty crazy. In #927, I changed

```js
concat(["(", join(concat([", "]), printed), ")"]),
```

into

```js
concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),
```

which makes the example in #1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.

In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.

The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.

The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.

I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

Fix #1203

vjeux added a commit to vjeux/prettier that referenced this pull request Apr 13, 2017

Fix edge cases triggered by newlines in arrow functions
This one is pretty crazy. In #927, I changed

```js
concat(["(", join(concat([", "]), printed), ")"]),
```

into

```js
concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),
```

which makes the example in #1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.

In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.

The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.

The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.

I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

Fix #1203

vjeux added a commit that referenced this pull request Apr 13, 2017

Fix edge cases triggered by newlines in arrow functions (#1217)
This one is pretty crazy. In #927, I changed

```js
concat(["(", join(concat([", "]), printed), ")"]),
```

into

```js
concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),
```

which makes the example in #1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.

In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.

The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.

The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.

I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1

Fix #1203
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment