Add heuristic to format functional composition more nicely by suchipi · Pull Request #4431 · prettier/prettier

suchipi · 2018-05-06T19:08:16Z

Based on discussion in #4172, this makes CallExpressions whose callee appears to be a functional composition function always break their arguments.

I added examples for ramda, rxjs, lodash, redux, and a generic "compose/composeFlipped".

Fixes #4172
Fixes #1420

lydell · 2018-05-06T19:15:06Z

src/language-js/printer-estree.js

+// composeP
+// composeK
+// flow
+// flowRight


Since you listed out all the possibilities anyway, perhaps we should make an array with all names and use that instead of a regex?

vjeux · 2018-05-06T19:15:32Z

src/language-js/printer-estree.js


+  const maybeTrailingComma = shouldPrintComma(options, "all") ? "," : "";
+
+  function allArgsBrokenOut() {


Don't create a function, just inline the content in the if

Ohh nevermind, you're using it below as well

vjeux · 2018-05-06T19:19:59Z

src/language-js/printer-estree.js

+  "composeK",
+  "flow",
+  "flowRight"
+];


If you are inlining it like this, then please use an object so it doesn't have to do a linear lookup every time.

{ pipe: true, pipeP: true, ... }

What about a Set?

(Btw, as far as I remember we have lots of such arrays where we do indexOf.)

vjeux · 2018-05-06T19:21:19Z

tests/functional_composition/functional_compose.js

+
+
+
+


why all the empty lines?

Because my editor was prettifying on save so I pasted these into nano 😅 I can remove them

vjeux · 2018-05-06T19:21:34Z

tests/functional_composition/lodash_flow_right.js

+
+foo(6);
+
+


vjeux · 2018-05-06T19:21:42Z

tests/functional_composition/lodash_flow.js

+
+foo(6);
+
+


vjeux · 2018-05-06T19:26:22Z

This issue has more examples. connect would likely be worthy of inclusion for redux.
#1420

vjeux · 2018-05-06T19:27:58Z

src/language-js/printer-estree.js

+  composeP: true,
+  composeK: true,
+  flow: true,
+  flowRight: true


Maybe add as comments which is coming from which library, so we know in the future where it came from

Also add comments for libraries

vjeux

Looks good to me, thanks! It's going to make a lot of people happy :)

suchipi · 2018-05-06T19:39:04Z

Thanks for coming up with the good heuristic @vjeux :)

adrianmcli · 2018-05-08T04:10:56Z

Awesome work all-around. Thanks y'all.

szkl · 2018-06-01T03:29:58Z

This issue has more examples. connect would likely be worthy of inclusion for redux.
#1420

What about the specified names occurring in different contexts? socket.connect for example.

suchipi · 2018-06-01T15:53:40Z

I avoided socket.pipe by only matching if there's more than one argument, but it looks like this is mistakenly hitting socket.connect in some cases:

Prettier 1.13.4
Playground link

--parser babylon

Input:

socket.connect(2000, () => {})

Output:

socket.connect(
  2000,
  () => {}
);

We could probably refine this to bail if any of the arguments are number, string, or object literals. Thoughts?

duailibe · 2018-06-01T15:57:13Z

@suchipi makes sense. Or whitelist for call expressions, identifiers, (arrow) functions

suchipi · 2018-06-01T16:06:22Z

I'd rather do an omit list than an allow list because you could have functions as object properties and access them with a member expression.

princed · 2018-06-21T12:39:06Z

@suchipi Is there any GH issue on that matter?
We got hit in our own classes with Prettier producing code like this:

this.compose(
  d1,
  d2
);

suchipi · 2018-06-21T15:18:11Z

@princed I haven't made one yet, if you could make one, I would appreciate it

princed · 2018-06-26T07:39:42Z

@suchipi Raised #4751

lxe · 2018-07-03T20:52:00Z

This is ~~clever~~ actually very nice for most cases, but I don't think that hardcoding function names from arbitrary libraries into a general purpose JS formatter is a good idea. Maybe there's a way to expand the heuristic to allow for configuration or use the argument types somehow?

suchipi · 2018-07-03T21:15:18Z

@lxe there is some discussion related to that problem space in #4424

shannonmoeller · 2018-07-03T22:49:25Z

I agree with @lxe. Hate to be a downer, but this change is awful. Adding a hardcoded list of function names to treat specially has just made prettier a React code formatter, not a general-purpose code formatter.

j-f1 · 2018-07-03T23:13:33Z

Adding a hardcoded list of function names to treat specially has just made prettier a React code formatter, not a general-purpose code formatter.

None of these changes cause the React APIs to be printed differently. Also, we already do some of this special-casing (for example, never breaking it() calls).

benlesh · 2018-07-20T20:16:46Z

Honestly, I think you'll be better off just doing what clang format did and using a trailing comma as the heuristic to decide if you should inline them all or put them each on their own lines:

someThing.whatever(foo, bar, baz, qux);

//vs

somthing.whatever(
  foo, 
  bar,
  baz,
  qux,
)

It's easy to control
It's easy to learn
It's fairly intuitive
It's great for git diffs
You don't have to write crazy logic guesswork algorithms to figure out what to do.

🤷‍♂️

j-f1 · 2018-07-20T21:22:28Z

Not all runtimes support trailing commas in functions, though.

suchipi · 2018-07-20T22:55:36Z

It's easy to control

This is a negative, from my perspective. For me, a big value of prettier is that I can let it "do it's thing" and be hands-off about formatting... at least, mostly. We rely on hinting for multilining object literals and I have to break out of my flow to adjust formatting from time to time because of it. I would rather it didn't let me influence the formatting at all- then I could focus on writing the code and let the formatting be what it is. But the ability to influence the formatting wedges itself into my workflow...

Add heuristic to format functional composition more nicely

f0b5edb

suchipi mentioned this pull request May 6, 2018

RxJS pipe chaining get formatted on a single line #4172

Closed

lydell reviewed May 6, 2018

View reviewed changes

vjeux reviewed May 6, 2018

View reviewed changes

Regex -> array

ee6924c

vjeux reviewed May 6, 2018

View reviewed changes

tests/functional_composition/lodash_flow_right.js Outdated

foo(6);

Copy link
Copy Markdown

Contributor

vjeux May 6, 2018

Choose a reason for hiding this comment

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

same here

vjeux reviewed May 6, 2018

View reviewed changes

tests/functional_composition/lodash_flow.js Outdated

foo(6);

Copy link
Copy Markdown

Contributor

vjeux May 6, 2018

Choose a reason for hiding this comment

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

and here

suchipi added 2 commits May 6, 2018 13:25

Use object instead of array for faster lookup

1ccf131

Remove empty lines

0b7b2d7

vjeux reviewed May 6, 2018

View reviewed changes

Add redux connect

eaaa41c

Also add comments for libraries

vjeux approved these changes May 6, 2018

View reviewed changes

duailibe approved these changes May 6, 2018

View reviewed changes

suchipi merged commit ed18f9f into prettier:master May 8, 2018

lipis added this to the 1.13 milestone May 9, 2018

lydell mentioned this pull request May 30, 2018

fix(javascript): regression for hasOwnProperty #4603

Merged

princed mentioned this pull request Jun 26, 2018

Functional composition heuristics false positives #4751

Closed

alextreppass mentioned this pull request Jul 12, 2018

Improve functional composition heuristics #4758

Closed

suchipi mentioned this pull request Aug 3, 2018

Strange behavior with word connect #4935

Closed

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Oct 18, 2018

lock bot locked as resolved and limited conversation to collaborators Oct 18, 2018


		const maybeTrailingComma = shouldPrintComma(options, "all") ? "," : "";

		function allArgsBrokenOut() {

Uh oh!

Conversation

suchipi commented May 6, 2018 • edited by vjeux Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suchipi May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux commented May 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vjeux left a comment

Choose a reason for hiding this comment

Uh oh!

suchipi commented May 6, 2018

Uh oh!

adrianmcli commented May 8, 2018

Uh oh!

szkl commented Jun 1, 2018

Uh oh!

suchipi commented Jun 1, 2018

Uh oh!

duailibe commented Jun 1, 2018

Uh oh!

suchipi commented Jun 1, 2018

Uh oh!

princed commented Jun 21, 2018

Uh oh!

suchipi commented Jun 21, 2018

Uh oh!

princed commented Jun 26, 2018

Uh oh!

lxe commented Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suchipi commented Jul 3, 2018

Uh oh!

shannonmoeller commented Jul 3, 2018

Uh oh!

j-f1 commented Jul 3, 2018

Uh oh!

benlesh commented Jul 20, 2018

Uh oh!

j-f1 commented Jul 20, 2018

Uh oh!

suchipi commented Jul 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

suchipi commented May 6, 2018 •

edited by vjeux

Loading

suchipi May 6, 2018 •

edited

Loading

lxe commented Jul 3, 2018 •

edited

Loading