JS: Break lines before binary operators (fixes #3806) #7111

btmills · 2019-12-09T05:43:01Z

On Friday, @evilebottnawi requested a PR to resolve #3806 in v2.0 by breaking lines, where necessary, before binary operators rather than after them, putting the operators at the beginning of each line.

Previously, there was special case logic to break lines before pipe operators but after all other operators. I removed the special case handling so that line breaks occur before all operators.

I left the implementation, tests, and changelog as separate commits to help ease review. Happy to squash or make any other changes as necessary to help get this in!

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.

✨Try the playground for this PR✨

alexander-akait · 2019-12-09T09:43:41Z

/cc @prettier/core will be great if you look at this

thorn0 · 2019-12-09T09:51:22Z

I thought there was a consensus that 2.0 would be focused on deprecating old Node versions etc. Updating to 2.0 should not lead to massive code reformatting. Otherwise too many people might prefer to keep using 1.x.

BrianHVB · 2019-12-09T16:38:57Z

The main 2.0 thread(s) have mainly talked about Node versions, but there have also been many comments on not introducing changes that there is disagreement or lack of consensus on.

But there seems to be a very strong consensus on the proposed fix to #3806. The only argument offered against the change is that it would be breaking and shouldn't be implemented in a minor version. However, for the change itself, there is widespread agreement both in the issue thread and in the JavaScript community.

AirBnB Issue: airbnb/javascript#1281
AirBnB Style Guidelines: https://github.com/airbnb/javascript#comparison--nested-ternaries

StandardJS (based on ESLint, widely used): https://standardjs.com/rules.html

thorn0 · 2019-12-10T00:13:25Z

@BrianHVB You completely miss the point of why the ex-2.0 was renamed to 3.0. I'm not saying that there is no consensus about this change, only that first things first.

changelog_unreleased/javascript/pr-7111.md

fisker · 2020-01-31T16:01:54Z

Anyone considered break line both before and after?

const isEventTarget = value != null
  && 
    typeof value.addEventListener === "function"
  && 
    typeof value.removeEventListener === "function"
  && 
    typeof value.dispatchEvent === "function"
  && 
    typeof value.dispatchEvent === "function"
  -
    Also_Nice_With_A_Shorter_Operator_Inside
  ?
    SOME_OTHER_EXPRESSION
  ??
    SOME_OTHER_EXPRESSION
  instanceof
    Even_Longer_Operator

btmills · 2020-02-03T06:02:20Z

I've rebased this change on next to resolve merge conflicts, added tests verifying that this change only moves binary operators to the beginning of lines while leaving assignment operators on their previous lines, and corrected the changelog in 300b900.

VincentLanglet · 2020-02-29T12:02:46Z

@btmills There is still a conflict

phaux · 2020-02-29T14:15:24Z

tests/logical_expressions/__snapshots__/jsfmt.spec.js.snap

+  (isAnyValueSelected
+    && node.getAttribute(radioAttr.toLowerCase()) === radioValue)
+  || (!isAnyValueSelected && values[a].default === true)
+  || a === 0;


Would it be possible to also change this case where the multiline operators are nested? When I encounter such a case in my code, I usually refactor it so that prettier doesn't make it ugly like that. If I was writing it manually I would write something like:

( isAnyValueSelected && node.getAttribute(radioAttr.toLowerCase()) === radioValue ) || ( !isAnyValueSelected && values[a].default === true ) || a === 0;

I'm trying to keep this change as small as possible to improve the odds the team decides to merge it. Based on the volume of 👍s on #3806, putting binary operators at the beginning of lines has pretty widespread support. It helps that the logic change to do that exclusively deletes code. I'm concerned that expanding the scope could reduce the chance it gets merged at all.

Sure. I didn't mean this comment to show as a "review" of this PR. It was just a random question 😅 This needs a separate issue.

btmills · 2020-03-02T17:55:34Z

@btmills There is still a conflict

@VincentLanglet I'm not surprised one of the snapshots has been updated since I last rebased. Rebasing the branch takes several minutes, so I haven't been actively monitoring this PR to keep it up to date. If the team indicates they're interested in merging, I'll certainly do another rebase as soon as possible!

btmills · 2020-03-23T03:55:31Z

I’ve rebased this on top of master now that 2.0 has been released.

tattali · 2020-07-13T09:40:26Z

Hi, it looks like the conflicting files are just snapshots and this for years.
Are there any plans to merge this pull request into a milestone?

There has been a lot of discussion around this PR I think it is a quite important one.

Thank you!

btmills · 2020-11-22T23:19:00Z

Rebased after the 2.2.0 release:

printBinaryishExpression() was extracted from src/language-js/printer-estree.js to src/language-js/print/binaryish.js, but the logic didn't change, so I just had to move the diff.
The changelog format appears to have changed since the last rebase, so that's now 7111.md instead of pr-7111.md.
No tests needed to be updated since the last rebase.

A demo install via prettier@npm:@btmills/prettier@^2.2.0 with yarn and npm v7 correctly adds node_modules/.bin/prettier. I don't have an npm v6 install handy right now to see if it's been fixed there.

alex12058 · 2022-02-12T02:19:18Z

This does not seem to handle comments correctly.

Input:

const logical_expression =
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa /* trailing comment */
  /* leading comment */ && bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
  // line comment
  && cccccccccccccccccccccccc

Output:

const logical_expression =
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa /* trailing comment */
  && /* leading comment */ bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb &&
  // line comment
  cccccccccccccccccccccccc;

Leading comments are placed after operators (which is fine) and operators do not appear at the start of the line when there is a line comment.

alex12058 · 2022-02-12T08:51:13Z

In src/language-js/print/binaryish.js if you replace this:

   right = [
      lineBeforeOperator ? line : "",
      operator,
      lineBeforeOperator ? " " : line,
      rightContent,
      rightSuffix,
    ];

with this:

    // Place leading own line comments before the operator.
    const comment = commentBeforeOperator ? rightContent.splice(0, 1)[0] : '';
    right = [
      line,
      comment,
      operator,
      " ",
      rightContent,
      rightSuffix,
    ];

it will extract leading own line comments from the rightContent and place them before the operator. commentBeforeOperator is the inversion of lineBeforeOperator.

Before changing operator positions, these tests demonstrate the current state.

Previously, there was special case logic to break lines before pipe operators but after all other operators. By removing the special case, line breaks, where necesary, will be placed before all operators.

btmills · 2022-04-13T07:18:50Z

Thank you for catching that bug, @alex12058, and thank you even more for suggesting the fix! I was able to use it with a modification to avoid clobbering type annotation comments.

I rebased this on latest main.

alex12058 · 2022-04-14T23:49:03Z

@btmills Good catch on the type annotation comments.

~~I found a potential formatting issue with type annotation comments that appear in multline binary operators.~~

// Input
var a =  veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar || /** @type {string} */
    (c);

// Output
var a =
  veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar
  || /** @type {string} */
  (c);

// Expected
var a =
  veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar
  || /** @type {string} */ (c);

playground link

What do you think the output should be?

Edit: I think the current output if is fine as it respects the users preference to place the comment on it's own line or inline.

// Input
var a =
  veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar
  || /** @type {string} */ (c);

var a =
  veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar
  || /** @type {string} */
  (c);

// Output
var a =
  veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar
  || /** @type {string} */ (c);

var a =
  veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar
  || /** @type {string} */
  (c);

btmills · 2022-04-23T20:46:48Z

@alex12058 I agree with your edit. That seems just far enough away as to be unrelated, so I'm glad this change doesn't affect behavior there either way.

JanJakes · 2022-09-08T08:50:19Z

Seeing Release v3 being planned, I'd like to ask whether this is considered for v3. I see it has the 3.0 milestone, but at the same time, it's not on the release checklist. It seems to be the most upvoted PR, issue (second, but almost no downvotes), and comment on the v3 issue. It seems to be non-controversial issue with a broad consensus. Thanks for any information!

fisker · 2023-02-08T10:10:55Z

I'm going to postpone this, since we haven't reached a consensus.

bsell93 · 2023-02-08T17:04:06Z

@fisker can you explain what you mean?

From what I can see there is a consensus it was just a matter of deciding which release to put it in (v2 or v3):
#7111 (comment)

sosukesuzuki · 2023-03-19T12:47:40Z

This is a major formatting change, so I think we should hide this feature in an experimental flag(Please see #14527 about experimental flags). Then it could be shipped in v3 as an experimental feature.

btmills · 2023-04-08T18:06:35Z

I'd love to see this merged in, even if it's only experimental initially! Since it sounds like that's likely to happen after v3, I'll wait until that's out and rebase this branch again.

btmills mentioned this pull request Dec 9, 2019

Placing operators at the beginning of lines #3806

Open

alexander-akait added this to the 2.0 milestone Dec 9, 2019

thorn0 reviewed Dec 10, 2019

View reviewed changes

changelog_unreleased/javascript/pr-7111.md Outdated Show resolved Hide resolved

VincentLanglet mentioned this pull request Jan 9, 2020

Prettier 2.0 – 2.0 2020 #6888

Closed

thorn0 modified the milestones: 2.0, 3.0 Jan 9, 2020

thorn0 force-pushed the next branch 2 times, most recently from e16f758 to 7d88fb8 Compare January 10, 2020 00:39

btmills force-pushed the issue3806 branch 3 times, most recently from ba9d939 to 300b900 Compare February 3, 2020 06:00

phaux reviewed Feb 29, 2020

View reviewed changes

thorn0 force-pushed the next branch from 55247de to 262423a Compare March 21, 2020 19:03

thorn0 closed this Mar 22, 2020

thorn0 reopened this Mar 22, 2020

thorn0 changed the base branch from next to master March 22, 2020 21:11

btmills force-pushed the issue3806 branch 2 times, most recently from f5f93ae to 8723d6b Compare March 22, 2020 23:12

btmills mentioned this pull request Aug 24, 2020

option to set && || at new line btmills/prettier#2

Closed

btmills force-pushed the issue3806 branch from 8723d6b to bec3247 Compare August 26, 2020 06:32

btmills force-pushed the issue3806 branch 2 times, most recently from b2d88d9 to a2f6eb6 Compare November 22, 2020 23:07

Base automatically changed from master to main January 23, 2021 17:13

btmills added 3 commits April 13, 2022 02:48

Add additional tests around operators

f41990d

Before changing operator positions, these tests demonstrate the current state.

JS: Break lines before binary operators (fixes prettier#3806)

082f125

Previously, there was special case logic to break lines before pipe operators but after all other operators. By removing the special case, line breaks, where necesary, will be placed before all operators.

Update tests for pre-operator line breaks

c463209

btmills force-pushed the issue3806 branch from a2f6eb6 to 1f66029 Compare April 13, 2022 06:49

Add changelog for pre-operator line breaks

c1de909

btmills force-pushed the issue3806 branch from 1f66029 to c1de909 Compare April 13, 2022 06:54

chrisbobbe mentioned this pull request Jul 20, 2022

Release v3 #13142

Closed

23 tasks

MarkAckert mentioned this pull request Nov 29, 2022

Eslint rule updates zowe/zowe-chat#178

Merged

15 tasks

fisker modified the milestones: 3.0, 4.0 Feb 8, 2023

sosukesuzuki mentioned this pull request Mar 19, 2023

Add experimentalOperatorPosition for breaking lines before binary operators #14537

Draft

4 tasks

jtkiesel mentioned this pull request Nov 30, 2023

Wrap before binary operators jhipster/prettier-java#534

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS: Break lines before binary operators (fixes #3806) #7111

JS: Break lines before binary operators (fixes #3806) #7111

btmills commented Dec 9, 2019

alexander-akait commented Dec 9, 2019

thorn0 commented Dec 9, 2019

BrianHVB commented Dec 9, 2019

thorn0 commented Dec 10, 2019

fisker commented Jan 31, 2020 •

edited

btmills commented Feb 3, 2020

VincentLanglet commented Feb 29, 2020

phaux Feb 29, 2020

btmills Mar 2, 2020

phaux Mar 2, 2020 •

edited

btmills commented Mar 2, 2020

btmills commented Mar 23, 2020

tattali commented Jul 13, 2020

btmills commented Nov 22, 2020

alex12058 commented Feb 12, 2022 •

edited

alex12058 commented Feb 12, 2022 •

edited

btmills commented Apr 13, 2022

alex12058 commented Apr 14, 2022 •

edited

btmills commented Apr 23, 2022

JanJakes commented Sep 8, 2022

fisker commented Feb 8, 2023

bsell93 commented Feb 8, 2023

sosukesuzuki commented Mar 19, 2023 •

edited

btmills commented Apr 8, 2023 •

edited

JS: Break lines before binary operators (fixes #3806) #7111

Are you sure you want to change the base?

JS: Break lines before binary operators (fixes #3806) #7111

Conversation

btmills commented Dec 9, 2019

alexander-akait commented Dec 9, 2019

thorn0 commented Dec 9, 2019

BrianHVB commented Dec 9, 2019

thorn0 commented Dec 10, 2019

fisker commented Jan 31, 2020 • edited

btmills commented Feb 3, 2020

VincentLanglet commented Feb 29, 2020

phaux Feb 29, 2020

Choose a reason for hiding this comment

btmills Mar 2, 2020

Choose a reason for hiding this comment

phaux Mar 2, 2020 • edited

Choose a reason for hiding this comment

btmills commented Mar 2, 2020

btmills commented Mar 23, 2020

tattali commented Jul 13, 2020

btmills commented Nov 22, 2020

alex12058 commented Feb 12, 2022 • edited

alex12058 commented Feb 12, 2022 • edited

btmills commented Apr 13, 2022

alex12058 commented Apr 14, 2022 • edited

btmills commented Apr 23, 2022

JanJakes commented Sep 8, 2022

fisker commented Feb 8, 2023

bsell93 commented Feb 8, 2023

sosukesuzuki commented Mar 19, 2023 • edited

btmills commented Apr 8, 2023 • edited

fisker commented Jan 31, 2020 •

edited

phaux Mar 2, 2020 •

edited

alex12058 commented Feb 12, 2022 •

edited

alex12058 commented Feb 12, 2022 •

edited

alex12058 commented Apr 14, 2022 •

edited

sosukesuzuki commented Mar 19, 2023 •

edited

btmills commented Apr 8, 2023 •

edited