Skip to content

Indent long ConditionalExpression in MemberExpression #7564

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.

Sign up for GitHub

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

Closed
fisker wants to merge 4 commits into from
Closed

Indent long ConditionalExpression in MemberExpression #7564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e5e329
Add fail tests
fisker Feb 8, 2020
5e30947
Attempt fix
fisker Feb 8, 2020
6d66f51
Add changelog
fisker Feb 8, 2020
842ce0e
Add test fixture from issue
fisker Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions changelog_unreleased/javascript/pr-7564.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#### Indent long `ConditionalExpression` in `MemberExpression` ([#7564](https://github.com/prettier/prettier/pull/7564) by [@fisker](https://github.com/fisker))

<!-- prettier-ignore -->
```js
// Input
(a.very.long.very.long.very.long.very.long.very.long.conditional.expression?foo:bar).baz;

// Prettier stable
(a.very.long.very.long.very.long.very.long.very.long.conditional.expression
? foo
: bar
).baz;

// Prettier master
(
a.very.long.very.long.very.long.very.long.very.long.conditional.expression
? foo
: bar
).baz;
```
19 changes: 8 additions & 11 deletions src/language-js/printer-estree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,6 @@ function printDecorators(path, options, print) {
* and TSConditionalType
* @typedef {Object} OperatorOptions
* @property {() => Array<string | Doc>} beforeParts - Parts to print before the `?`.
* @property {(breakClosingParen: boolean) => Array<string | Doc>} afterParts - Parts to print after the conditional expression.
* @property {boolean} shouldCheckJsx - Whether to check for and print in JSX mode.
* @property {string} conditionalNodeType - The type of the conditional expression node, ie "ConditionalExpression" or "TSConditionalType".
* @property {string} consequentNodePropertyName - The property at which the consequent node can be found on the main node, eg "consequent".
Expand Down Expand Up @@ -394,12 +393,13 @@ function printTernaryOperator(path, options, print, operatorOptions) {
const maybeGroup = doc =>
parent === firstNonConditionalParent ? group(doc) : doc;

// Break the closing paren to keep the chain right after it:
// (a
// ? b
// : c
// Break the paren to keep the chain right after it:
// (
// a
// ? b
// : c
// ).call()
const breakClosingParen =
const breakParen =
!jsxMode &&
(parent.type === "MemberExpression" ||
parent.type === "OptionalMemberExpression" ||
Expand All @@ -424,13 +424,12 @@ function printTernaryOperator(path, options, print, operatorOptions) {
parent[operatorOptions.alternateNodePropertyName] === node
? align(2, testDoc)
: testDoc)(concat(operatorOptions.beforeParts())),
forceNoIndent ? concat(parts) : indent(concat(parts)),
operatorOptions.afterParts(breakClosingParen)
forceNoIndent ? concat(parts) : indent(concat(parts))
)
)
);

return isParentTest
return isParentTest || breakParen
? group(concat([indent(concat([softline, result])), softline]))
: result;
}
Expand Down Expand Up @@ -1695,7 +1694,6 @@ function printPathNoParens(path, options, print, args) {
case "ConditionalExpression":
return printTernaryOperator(path, options, print, {
beforeParts: () => [path.call(print, "test")],
afterParts: breakClosingParen => [breakClosingParen ? softline : ""],
shouldCheckJsx: true,
conditionalNodeType: "ConditionalExpression",
consequentNodePropertyName: "consequent",
Expand Down Expand Up @@ -3562,7 +3560,6 @@ function printPathNoParens(path, options, print, args) {
" ",
path.call(print, "extendsType")
],
afterParts: () => [],
shouldCheckJsx: false,
conditionalNodeType: "TSConditionalType",
consequentNodePropertyName: "trueType",
Expand Down
21 changes: 12 additions & 9 deletions tests/angular_interpolation/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,10 @@ trailingComma: "none"
keyB: shortValue | pipeB | pipeC: valueToPipeC
}
| aaa,
(hideLinqPanel
? "ReportSelection.HideShowLabel_Show.String"
: "ReportSelection.HideShowLabel_Hide.String"
(
hideLinqPanel
? "ReportSelection.HideShowLabel_Show.String"
: "ReportSelection.HideShowLabel_Hide.String"
) | localize: localizationSection
]
================================================================================
Expand Down Expand Up @@ -187,9 +188,10 @@ printWidth: 80
keyB: shortValue | pipeB | pipeC: valueToPipeC
}
| aaa,
(hideLinqPanel
? "ReportSelection.HideShowLabel_Show.String"
: "ReportSelection.HideShowLabel_Hide.String"
(
hideLinqPanel
? "ReportSelection.HideShowLabel_Show.String"
: "ReportSelection.HideShowLabel_Hide.String"
) | localize: localizationSection
]
================================================================================
Expand Down Expand Up @@ -254,9 +256,10 @@ trailingComma: "all"
keyB: shortValue | pipeB | pipeC: valueToPipeC
}
| aaa,
(hideLinqPanel
? "ReportSelection.HideShowLabel_Show.String"
: "ReportSelection.HideShowLabel_Hide.String"
(
hideLinqPanel
? "ReportSelection.HideShowLabel_Show.String"
: "ReportSelection.HideShowLabel_Hide.String"
) | localize: localizationSection
]
================================================================================
Expand Down
7 changes: 4 additions & 3 deletions tests/member/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ printWidth: 80
.prop;

=====================================output=====================================
(valid
? helper.responseBody(this.currentUser)
: helper.responseBody(this.defaultUser)
(
valid
? helper.responseBody(this.currentUser)
: helper.responseBody(this.defaultUser)
).prop;

================================================================================
Expand Down
21 changes: 12 additions & 9 deletions tests/method-chain/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,21 +507,24 @@ object[valid

(a ? b : c).d().e().f();

(valid
? helper.responseBody(this.currentUser)
: helper.responseBody(this.defaultUser)
(
valid
? helper.responseBody(this.currentUser)
: helper.responseBody(this.defaultUser)
).map();

(valid
? helper.responseBody(this.currentUser)
: helper.responseBody(this.defaultUser)
(
valid
? helper.responseBody(this.currentUser)
: helper.responseBody(this.defaultUser)
)
.map()
.filter();

(valid
? helper.responseBody(this.currentUser)
: helper.responseBody(defaultUser)
(
valid
? helper.responseBody(this.currentUser)
: helper.responseBody(defaultUser)
Copy link
Copy Markdown
Member

@thorn0 thorn0 Feb 13, 2020

Choose a reason for hiding this comment

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

Do we really want to change the formatting in these simpler cases where the condition fits on one line?

Copy link
Copy Markdown
Member Author

@fisker fisker Feb 13, 2020

Choose a reason for hiding this comment

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

You mean this?

Copy link
Copy Markdown
Member Author

@fisker fisker Feb 13, 2020

Choose a reason for hiding this comment

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

Seems I didn't change much, only difference is one line after (.

#7564

stable

Copy link
Copy Markdown
Member

@thorn0 thorn0 Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I mean this. Looks more questionable when it's part of expression (stable).

Copy link
Copy Markdown
Member

@thorn0 thorn0 Feb 14, 2020

Choose a reason for hiding this comment

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

Also this problem exists in return statements. Can be solved by adding parens. Or it's probably better to find a generic way to increase the indentation of ? and : if the condition (the part before ?) doesn't fit on one line.

Copy link
Copy Markdown
Member Author

@fisker fisker Feb 14, 2020

Choose a reason for hiding this comment

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

Personal I prefer new one, we are not running + with the ternary, we are calculating with (ternary).foo

Copy link
Copy Markdown
Member Author

@fisker fisker Feb 14, 2020

Choose a reason for hiding this comment

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

Like this?

function a() {
  return (
      valid === getSomethingFromSomeWhere() || this.currentUser === 'Foobert Barberg'
    )
    ? valid === getSomethingFromSomeWhere() ||
       this.currentUser === "Foobert Barberg"
    : valid === getSomethingFromSomeWhere() ||
       this.currentUser === "Foobert Barberg"
}

This seems weird....

Copy link
Copy Markdown
Member

@thorn0 thorn0 Feb 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That's what I meant:

function oneLineCondition() {
  return valid === getSomethingFromSomeWhere()
    ? helper.responseBody(this.currentUser)
    : helper.responseBody(this.defaultUser);
}

function twoLineCondition() {
  return valid === getSomethingFromSomeWhere() ||
    this.currentUser === "Foobert Barberg"
      ? helper.responseBody(this.currentUser)
      : helper.responseBody(this.defaultUser);
}

I propose that the indentation level of the second and the third arguments of the conditional operator should depend on how many lines the first argument takes.

).map();

object[
Expand Down
74 changes: 74 additions & 0 deletions tests/return/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,77 @@ fn(function f() {

================================================================================
`;

exports[`conditional.js 1`] = `
====================================options=====================================
parsers: ["flow", "typescript"]
printWidth: 80
| printWidth
=====================================input======================================
function foo (){
return (
a
? looooooooooooooooooooooooooooooooooooooooooooooooooong1
: looooooooooooooooooooooooooooooooooooooooooooooooooong2
);
}

function foo (){
return (
a
? looooooooooooooooooooooooooooooooooooooooooooooooooong1
: looooooooooooooooooooooooooooooooooooooooooooooooooong2
).foo;
}

function foo (){
return (
a
? looooooooooooooooooooooooooooooooooooooooooooooooooong1
: looooooooooooooooooooooooooooooooooooooooooooooooooong2
).foo();
}

// #4976
const decorated = ((arg, ignoreRequestError) => {
return (
typeof arg === 'string' || (arg && arg.valueOf && typeof arg.valueOf() === 'string')
? $delegate(arg, ignoreRequestError)
: handleAsyncOperations(arg, ignoreRequestError)).foo();
});

=====================================output=====================================
function foo() {
return a
? looooooooooooooooooooooooooooooooooooooooooooooooooong1
: looooooooooooooooooooooooooooooooooooooooooooooooooong2;
}

function foo() {
return (
a
? looooooooooooooooooooooooooooooooooooooooooooooooooong1
: looooooooooooooooooooooooooooooooooooooooooooooooooong2
).foo;
}

function foo() {
return (
a
? looooooooooooooooooooooooooooooooooooooooooooooooooong1
: looooooooooooooooooooooooooooooooooooooooooooooooooong2
).foo();
}

// #4976
const decorated = (arg, ignoreRequestError) => {
return (
typeof arg === "string" ||
(arg && arg.valueOf && typeof arg.valueOf() === "string")
? $delegate(arg, ignoreRequestError)
: handleAsyncOperations(arg, ignoreRequestError)
).foo();
};

================================================================================
`;
31 changes: 31 additions & 0 deletions tests/return/conditional.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
function foo (){
return (
a
? looooooooooooooooooooooooooooooooooooooooooooooooooong1
: looooooooooooooooooooooooooooooooooooooooooooooooooong2
);
}

function foo (){
return (
a
? looooooooooooooooooooooooooooooooooooooooooooooooooong1
: looooooooooooooooooooooooooooooooooooooooooooooooooong2
).foo;
}

function foo (){
return (
a
? looooooooooooooooooooooooooooooooooooooooooooooooooong1
: looooooooooooooooooooooooooooooooooooooooooooooooooong2
).foo();
}

// #4976
const decorated = ((arg, ignoreRequestError) => {
return (
typeof arg === 'string' || (arg && arg.valueOf && typeof arg.valueOf() === 'string')
? $delegate(arg, ignoreRequestError)
: handleAsyncOperations(arg, ignoreRequestError)).foo();
});