Add softline to assignment of binary expressions #871

Merged
merged 1 commit into from Mar 15, 2017

Conversation

Projects
None yet
2 participants
@vjeux
Collaborator

vjeux commented Mar 3, 2017

Printing the first line of a binary expression next to the = leads to weird cases where the first expression is parenthesised and doens't read well with chained conditionals as they don't align well. This makes it behave the same was as if tests.

Fixes #863

[RFC] Add softline to assignment of binary expressions
Printing the first line of a binary expression next to the `=` leads to weird cases where the first expression is parenthesised and doens't read well with chained conditionals as they don't align well. This makes it behave the same was as `if` tests.

Fixes #863
@@ -2802,6 +2790,34 @@ function printBinaryishExpressions(path, parts, print, options, isNested) {
return parts;
}
+function printAssignment(printedLeft, operator, rightNode, printedRight, options) {

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Collaborator

We copy and pasted this non trivial logic twice. I extracted it into its own function

@vjeux

vjeux Mar 3, 2017

Collaborator

We copy and pasted this non trivial logic twice. I extracted it into its own function

+ options.tabWidth,
+ concat([hardline, printedRight])
+ );
+ } else if (isBinaryish(rightNode) && !shouldInlineLogicalExpression(rightNode)) {

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Collaborator

this is the only condition I added in this PR

@vjeux

vjeux Mar 3, 2017

Collaborator

this is the only condition I added in this PR

+ descriptionLines;
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+const computedDescriptionLines =
+ (showConfirm && descriptionLinesConfirming) ||

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Collaborator

yay, this is now very readable!

@vjeux

vjeux Mar 3, 2017

Collaborator

yay, this is now very readable!

+ descriptionLines;
+
+computedDescriptionLines =
+ (focused && !loading && descriptionLinesFocused) || descriptionLines;

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Collaborator

This is the only part i'm not sure about, it would have been nice if || was breaking I guess.

@vjeux

vjeux Mar 3, 2017

Collaborator

This is the only part i'm not sure about, it would have been nice if || was breaking I guess.

@@ -85,6 +86,7 @@ var fnString = // Comment
// Comment
\\"some\\" + \\"long\\" + \\"string\\";
-var fnString = \\"some\\" + \\"long\\" + \\"string\\"; // Comment
+var fnString = // Comment

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Collaborator

This is actually how the string was printed in the first place :)

@vjeux

vjeux Mar 3, 2017

Collaborator

This is actually how the string was printed in the first place :)

- dependencyWithOutRelativePath.split(\\"/\\")[0]
-) !== -1;
+const isPartOfPackageJSON =
+ dependenciesArray.indexOf(dependencyWithOutRelativePath.split(\\"/\\")[0]) !== -1;

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Collaborator

this looks less weird.

@vjeux

vjeux Mar 3, 2017

Collaborator

this looks less weird.

longVariable &&
longVariable &&
longVariable &&
longVariable &&
longVariable;
-const x = (longVariable && longVariable) ||
+const x =
+ (longVariable && longVariable) ||

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Collaborator

yay alignment!

@vjeux

vjeux Mar 3, 2017

Collaborator

yay alignment!

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Mar 3, 2017

Member

This doesn't solve the issue. There are several other cases where the expression might have parens. One that quickly comes to mind is after your PR #862, we will add parens here:

let y;
const x = (y =
  111111111 +
  222222222 +
  33333333 +
  111111111 +
  222222222 +
  33333333);

This is the output with this branch rebased with your other PR.

I don't think we need to unconditionally bump down binary expressions. I guess we need to somehow figure out if the first expression requires parenthesis and only do it then.

Member

jlongster commented Mar 3, 2017

This doesn't solve the issue. There are several other cases where the expression might have parens. One that quickly comes to mind is after your PR #862, we will add parens here:

let y;
const x = (y =
  111111111 +
  222222222 +
  33333333 +
  111111111 +
  222222222 +
  33333333);

This is the output with this branch rebased with your other PR.

I don't think we need to unconditionally bump down binary expressions. I guess we need to somehow figure out if the first expression requires parenthesis and only do it then.

@jlongster jlongster changed the title from [RFC] Add softline to assignment of binary expressions to Add softline to assignment of binary expressions Mar 15, 2017

@vjeux vjeux merged commit 7105f97 into prettier:master Mar 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment