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

RFC: break method chains in assignments if they don't fit after operator #10297

Closed
wants to merge 1 commit into from

Conversation

thorn0
Copy link
Member

@thorn0 thorn0 commented Feb 11, 2021

Description

Proposal: if the right-hand side of an assignment is a member chain, then:

  • If the entire assignment fits on one line, keep it that way.
  • If the member chain doesn't fit after the operator, it's moved to the next line and is always split onto multiple lines at that.

Input:

const kochabCooieGameOnOboleUnweaveWayWay = someLongString.split("jest version =").pop().split(EOL)[0].trim();

Current output (playground):

const kochabCooieGameOnOboleUnweaveWayWay = someLongString
  .split("jest version =")
  .pop()
  .split(EOL)[0]
  .trim();

Output, this PR (playground):

const kochabCooieGameOnOboleUnweaveWayWay =
  someLongString
    .split("jest version =")
    .pop()
    .split(EOL)[0]
    .trim();

Alternative output (should we prefer it probably?):

const kochabCooieGameOnOboleUnweaveWayWay =
  someLongString.split("jest version =").pop().split(EOL)[0].trim();

I like this idea, but 1) I somewhat dislike the implementation (this is just a PoC, I'll refactor/fix it if we decide to go this way), 2) would be good to hear opinions.

Checklist

  • 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/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@brodybits
Copy link
Contributor

Break after = as in this proposal strikes me as inconsistent with the "fill" behavior that normally comes after =. In comparison:

Prettier pr-10297
Playground link

--parser babel

Input:

const kochabCooieGameOnOboleUnweaveWayWay = reduce(someLongString.split("jest version =").pop().split(EOL)[0].trim())

Output:

const kochabCooieGameOnOboleUnweaveWayWay = reduce(
  someLongString.split("jest version =").pop().split(EOL)[0].trim()
);

Of course we should still do break after = if absolutely needed, as PR #10222 does seem to get right:

Prettier pr-10222
Playground link

--parser babel

Input:

const kochabCooieGameOnOboleUnweaveWayWay = someLoooooooooooooooooooooooongerString.split("jest version =").pop().split(EOL)[0].trim();

Output:

const kochabCooieGameOnOboleUnweaveWayWay =
  someLoooooooooooooooooooooooongerString
    .split("jest version =")
    .pop()
    .split(EOL)[0]
    .trim();

@thorn0
Copy link
Member Author

thorn0 commented Feb 11, 2021

@brodybits I'm trying to avoid splitting the chain visually into two pieces:

image

BTW, your example with reduce would suffer from that too if its argument was shorter. There is an issue about this: #5610

@lipis
Copy link
Member

lipis commented Feb 11, 2021

I like the output in this PR and the alternative.. (I prefer more lines more though)

@thorn0
Copy link
Member Author

thorn0 commented Feb 11, 2021

@lipis Have also a look at the diff this PR would produce in real-world code:
prettier/prettier-regression-testing#27 (comment)

# Conflicts:
#	src/language-js/print/assignment.js
@fisker
Copy link
Member

fisker commented Feb 17, 2021

Not a fan of this style. I stay neutral on this.

@thorn0
Copy link
Member Author

thorn0 commented Feb 17, 2021

@fisker Do you prefer the current or alternative output? #10297 (comment)

@bobbyoz

This comment has been minimized.

@fisker
Copy link
Member

fisker commented Feb 17, 2021

current one, main reason is we can't write

return
  someLoooooooooooooooooooooooongerString
    .split("jest version =")
    .pop()
    .split(EOL)[0]
    .trim();

@thorn0
Copy link
Member Author

thorn0 commented Feb 17, 2021

return doesn't have this problem

@fisker
Copy link
Member

fisker commented Feb 18, 2021

I don't know, I'm just saying I won't write in this way myself.

BTW, if I have to choose between alternative one and this one, I choose this one.

@thorn0
Copy link
Member Author

thorn0 commented Feb 18, 2021

The alternative output now (after #10222) happens when the LHS is so long that even the head of the chain doesn't fit on the first line. This PR naturally solves that too. But we can consider solving only that case instead.

On the other hand, the alternative output looks definitely bad specifically for this chain only because the general method chain breaking heuristic should be improved. A mixed chain of this length with both (...) and [...] shouldn't stay unbroken, and this is not specific to assignments

@thorn0
Copy link
Member Author

thorn0 commented Feb 23, 2021

Closing. The method chain breaking heuristic should be improved instead.

@thorn0 thorn0 closed this Feb 23, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2022
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.

None yet

5 participants