Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Further improve method chain breaking heuristic #7889

Merged
merged 1 commit into from Apr 15, 2020

Conversation

thorn0
Copy link
Member

@thorn0 thorn0 commented Mar 27, 2020

fixes #3594
fixes #3621

A continuation of the work started here: #6685

A method chain now is always split onto multiple lines if:

  • it's an expression statement and all its arguments are literals or UPPERCASE_CONSTANTs (the "fluent configuration" pattern),
  • any call but the first one has more than two arguments, or
  • the chain starts with a constructor call.

Try the playground for this PR

To install it locally, run npm install thorn0/prettier#tweak-method-chains

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

@sosukesuzuki
Copy link
Member

oh playground link not found....

sosukesuzuki
sosukesuzuki previously approved these changes Mar 27, 2020
Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

looks good

@thorn0
Copy link
Member Author

thorn0 commented Mar 27, 2020

Should we probably check the total amount of arguments instead (>= 6)?
This hardly deserves splitting:

a = foo.bar("netlify", "what happened", "to previews?").baz().qux();

@sosukesuzuki
Copy link
Member

sosukesuzuki commented Mar 27, 2020

sounds good, but I think 6 is too much.
I want this code to be broken:

a = foo.bar("netlify", "aaaaaaa").baz("fooaaoooo", "baraaaarr", "bar").qux();

@sosukesuzuki sosukesuzuki dismissed their stale review March 28, 2020 00:01

I missed some behaviors

@thorn0
Copy link
Member Author

thorn0 commented Mar 28, 2020

"at least one call with more than two args" && "at least one other call with more than one arg"?

Or maybe: "at least one call with more than two args, but this call shouldn't be first in the chain"?

@sosukesuzuki
Copy link
Member

sounds good. if you push the implementation for it, I'll try it with various codes.

@thorn0
Copy link
Member Author

thorn0 commented Mar 28, 2020

pushed "at least one call with more than two args, but this call shouldn't be first in the chain"

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

What about this? I found this when I ran Prettier 2 on a project in my company. It looks ugly for me.
Playground
Input:

cy.get(".ready")
  .should("have.text", "FOO")
  .should({ have, color, aaa });

Output:

cy.get(".ready").should("have.text", "FOO").should({ have, color, aaa });

Expected(same as input):

cy.get(".ready")
  .should("have.text", "FOO")
  .should({ have, color, aaa });

@thorn0
Copy link
Member Author

thorn0 commented Mar 29, 2020

@sosukesuzuki My last take away from #7884 is that we probably should always break chains used as expression statements. What do you think about it?

@sosukesuzuki
Copy link
Member

@thorn0 Sorry if I make any mistake. "always break chains used as expression statements" means it breaks this code, right?

// Input
foo.bar().baz();

// Output?
foo
  .bar()
  .baz();

It looks overkill for me...

@thorn0
Copy link
Member Author

thorn0 commented Mar 29, 2020

@sosukesuzuki Do you have examples of real code that would look bad if we used this rule?

@sosukesuzuki
Copy link
Member

@thorn0 hmm, I have no idea. I got it, such code may not be used much in the real world.

@thorn0 thorn0 changed the title never print a chain on one line if it has a call with > 2 args Further improve method chain breaking heuristic Mar 29, 2020
@thorn0 thorn0 force-pushed the tweak-method-chains branch 5 times, most recently from 39bd9e7 to 8bb18db Compare March 30, 2020 18:02
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good idea

Comment on lines +573 to 597
const myScale = d3.scaleLinear()
.domain([1950, 1980])
.range([0, width])
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep the old one, add a new one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the old one was not real-life code. That's not how d3 is used. Our heuristics are developed to format real code. Ideally all the foo-bar-baz should be removed from that folder and replaced with more reasonable examples.

@fisker
Copy link
Member

fisker commented Apr 1, 2020

Realworld case of #7889 (comment)

array.flat().reverse()

Many methods on array can be called without arguments, .slice() to copy, .join() to join with ,, .sort(), .keys() / .entries() on last call. .shift() / .pop() get another type and then chain, objectsArray.pop().toString()

@thorn0
Copy link
Member Author

thorn0 commented Apr 1, 2020

@fisker In real code, the result of such array operations is usually used somehow, not discarded. E.g. result = array.flat().reverse();. Anyways, I've tweaked that part a bit, and now such simple chains without arguments aren't split even if they're expression statements. Please play with the playground to check how it works.

@thorn0 thorn0 force-pushed the tweak-method-chains branch 2 times, most recently from e96e597 to a9bad4c Compare April 1, 2020 21:31
@travislwatson
Copy link

travislwatson commented Apr 8, 2020

@thorn0 After I commented on #7884, I kept mulling over what a heuristic would be that made me not care about intentional line breaks, and after 2 days I think I've found it:

Never line-break inside a method chain before breaking the method chain itself.

I have extracted and hopefully simplified a few examples here.

First, line breaks inside an arrow function:

const Profile = view.with({ name }).as((props) => (
  <div>
    <h1>Hello, {props.name}</h1>
  </div>
))

// Better:
const Profile = view
  .with({ name })
  .as((props) => (
    <div>
      <h1>Hello, {props.name}</h1>
    </div>
  ))

Second, line breaks inside an object definition:

const fullName = RR.createQuery((o) => `${o.firstName} ${o.lastName}`).with({
  firstName,
  lastName,
})

// Better:
const fullName = RR
  .createQuery((o) => `${o.firstName} ${o.lastName}`)
  .with({ firstName, lastName })

Or most egregiously, line breaks inside an method call:

const [firstName, setFirstName] = RR.name('user.profile.firstName').createState(
  'John'
)

// Better
const [firstName, setFirstName] = RR
  .name('user.profile.firstName')
  .createState('John')

Hopefully this is a useful heuristic for this scenario.

@thorn0
Copy link
Member Author

thorn0 commented Apr 14, 2020

@traviswatsonws A very good point, thanks. As for your first example (with a function), it's unlikely we'll change how it's formatted because the current output is intended (#469 (review), #469 (review)), but the other two indeed can be improved as you wrote. Implementation-wise, it might be challenging, and not in this PR maybe, but I'll look into it. For your second example, we also need to accept and implement #8055 first.

@travislwatson
Copy link

travislwatson commented Apr 14, 2020

@thorn0 Would this discussion be better had in a different issue?

As for the first function example, that case actually makes sense to me, and my example as provided is actually on the acceptable side, with the exception of indentation:

EDIT: I don't know enough about this indentation stuff, I probably shouldn't have even mentioned it, but I'm going to leave it here for posterity. Maybe just... skip over it ;-)

// This feels normal to not be indented
items.map(mapFunction).forEach((item) => {
  // ...
})

// This feels awkward to not be indented, especially because if we add just one more step...
const output = items.map(mapFunction).map((item) => {
  // ...
})

// Then we get this chain treatment, which is indented
const output = items
  .map(mapFunction)
  .map((item) => {
    // ...
  })
  .map(otherMapFunction)

Both are chains, just one more step, so the inconsistency feels "off".

All that said, if I'm hypothesizing, that "break inside the lambda" treatment feels quite unique, and has some constraints:

  • There is only a single lambda in the chain
  • The lambda argument is a top level argument; it's not inside an array or object definition
  • The lambda argument is the last step of the chain (I think this is how it's behaving now, based on my testing)

And all this only matters when we are over the print width. I think these constraints are consistent with the link you provided.

Here is an example that feels awkward/confusing to me for the reasons above:

// Has 2 lambdas, and one of them is inside an object definition
const Profile = view.with({ name: (state) => state.name }).as((props) => (
  <div>
    <h1>Hello, {props.name}</h1>
  </div>
))

@thorn0
Copy link
Member Author

thorn0 commented Apr 14, 2020

The lambda argument is a top level argument; it's not inside an array or object definition
Here is an example that feels awkward/confusing to me for the reasons above:

@traviswatsonws Good catch. I'll fix this one in this PR. As for the rest, yes, we need to open a new issue.

@thorn0
Copy link
Member Author

thorn0 commented Apr 15, 2020

@traviswatsonws I've tried to implement your suggestion. Please play with this PR (playground) and tell me what you think. To install it locally, run npm install thorn0/prettier#tweak-method-chains

@travislwatson
Copy link

I like it @thorn0. It's not as verbose as what I had in my head, but playing around with different argument amounts and lengths feels pretty good.

👍

@thorn0
Copy link
Member Author

thorn0 commented Apr 15, 2020

It's not ready for merging yet. Committed it mostly to get feedback. I might move these last changes to another PR because they need refactoring and further tweaking.

E.g. there is this idempotence issue:

Prettier pr-8063
Playground link

--parser babel

Input:

export default function theFunction(action$, store) {
  return action$.ofType(THE_ACTION).switchMap(action => Observable
    .webSocket({ url: THE_URL,
      more: stuff(),
      evenMore: stuff({ value1: true, value2: false, value3: false }),
    })
    .filter(data => theFilter(data))
    .map(({ theType, ...data }) => theMap(theType, data))
    .retryWhen(errors => errors));
}

Output:

export default function theFunction(action$, store) {
  return action$.ofType(THE_ACTION).switchMap((action) =>
    Observable
      .webSocket({
        url: THE_URL,
        more: stuff(),
        evenMore: stuff({ value1: true, value2: false, value3: false }),
      })
      .filter((data) => theFilter(data))
      .map(({ theType, ...data }) => theMap(theType, data))
      .retryWhen((errors) => errors)
  );
}

Second Output:

export default function theFunction(action$, store) {
  return action$.ofType(THE_ACTION).switchMap((action) =>
    Observable.webSocket({
      url: THE_URL,
      more: stuff(),
      evenMore: stuff({ value1: true, value2: false, value3: false }),
    })
      .filter((data) => theFilter(data))
      .map(({ theType, ...data }) => theMap(theType, data))
      .retryWhen((errors) => errors)
  );
}

@thorn0 thorn0 added status:wip Pull requests that are a work in progress and should not be merged feedback wanted labels Apr 15, 2020
@thorn0
Copy link
Member Author

thorn0 commented Apr 15, 2020

I'm reverting these changes, moving them to a separate PR: #8063.

I've reverted the current PR to the stage where it was approved + a fix for a bug described in #7889 (comment). Let's merge this first.

@thorn0 thorn0 removed feedback wanted status:wip Pull requests that are a work in progress and should not be merged labels Apr 15, 2020
@thorn0 thorn0 merged commit f67f650 into prettier:master Apr 15, 2020
@thorn0 thorn0 deleted the tweak-method-chains branch April 15, 2020 21:09
@fisker fisker mentioned this pull request Jul 14, 2020
4 tasks
sosukesuzuki added a commit to sosukesuzuki/prettier that referenced this pull request Jul 29, 2020
sosukesuzuki added a commit to sosukesuzuki/prettier that referenced this pull request Jul 29, 2020
@sosukesuzuki sosukesuzuki mentioned this pull request Jul 29, 2020
2 tasks
sosukesuzuki added a commit that referenced this pull request Aug 19, 2020
* Revert #7889

* Remove changelog

* Run prettier
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2021
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.

Keep short promise chains indented Excessive line breaks for a short comparison expression
5 participants