Consistent indentations for conditional operators by sosukesuzuki · Pull Request #10187 · prettier/prettier

sosukesuzuki · 2021-01-31T19:19:57Z

Description

Fixes #8840
Fixes #4976

Checklist

I’ve added tests to confirm my change works.
(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✨

changelog_unreleased/typescript/10187.md

Co-authored-by: fisker Cheung <[email protected]>

…nto fix-8840

thorn0 · 2021-02-02T17:09:22Z

What about binary expressions? I don't think this change is intended for this PR.

Prettier pr-10187
Playground link

Output:

bifornCringerMoshedPerplexSawder =
  askTrovenaBeenaDependsRowans +
  ((
    glimseGlyphsHazardNoopsTieTie === 0
      ? averredBathersBoxroomBuggyNurl
      : anodyneCondosMalateOverateRetinol
  ) as AnnularCooeedSplicesWalksWayWay);

bifornCringerMoshedPerplexSawder =
  askTrovenaBeenaDependsRowans +
  ((
    glimseGlyphsHazardNoopsTieTie === 0 &&
    kochabCooieGameOnOboleUnweave === Math.PI
      ? averredBathersBoxroomBuggyNurl
      : anodyneCondosMalateOverateRetinol
  ) as AnnularCooeedSplicesWalksWayWay);

Prettier 2.2.1
Playground link

Output:

bifornCringerMoshedPerplexSawder =
  askTrovenaBeenaDependsRowans +
  ((glimseGlyphsHazardNoopsTieTie === 0
    ? averredBathersBoxroomBuggyNurl
    : anodyneCondosMalateOverateRetinol) as AnnularCooeedSplicesWalksWayWay);

bifornCringerMoshedPerplexSawder =
  askTrovenaBeenaDependsRowans +
  ((glimseGlyphsHazardNoopsTieTie === 0 &&
  kochabCooieGameOnOboleUnweave === Math.PI
    ? averredBathersBoxroomBuggyNurl
    : anodyneCondosMalateOverateRetinol) as AnnularCooeedSplicesWalksWayWay);

thorn0 · 2021-02-02T17:59:12Z

src/language-js/print/ternary.js

+  ) {
+    let memberChainingRootNodeName = parentName;
+    let count = 0;
+    while (memberChainingRootNodeName === "object") {


Member chains also include calls:
Prettier pr-10187
Playground link

--parser typescript

Input:

bifornCringerMoshedPerplexSawder = ( glimseGlyphsHazardNoopsTieTie === 0 && kochabCooieGameOnOboleUnweave === Math.PI ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol ).annularCooeedSplicesWalksWayWay.annularCooeedSplicesWalksWayWay( annularCooeedSplicesWalksWayWay ).annularCooeedSplicesWalksWayWay();

Output:

bifornCringerMoshedPerplexSawder = (glimseGlyphsHazardNoopsTieTie === 0 && kochabCooieGameOnOboleUnweave === Math.PI ? averredBathersBoxroomBuggyNurl : anodyneCondosMalateOverateRetinol ).annularCooeedSplicesWalksWayWay .annularCooeedSplicesWalksWayWay(annularCooeedSplicesWalksWayWay) .annularCooeedSplicesWalksWayWay();

thorn0 · 2021-02-03T20:17:28Z

Also member chains might include TSNonNullExpression.

fisker · 2021-02-04T02:04:48Z

src/language-js/print/ternary.js

  return printed;
 }

+const rightSideNodeNamesSet = new Set(["right", "init", "argument"]);


If I didn't misunderstand this, we are checking NodeA is NodeB's .right or .init, but not checking NodeB's type. I don't like this part, this may lead to unexpect cases.

For example argument, I guess you mean to check argument of ReturnStatement/YieldExpression etc, let's list them.

fisker · 2021-02-04T02:07:55Z

src/language-js/print/ternary.js

+   * )(arguments);
+   */
+  if (
+    parent.type === "CallExpression" &&


OptionalCallExpression too?

Prettier pr-10187
Playground link

--parser babel

Input:

function foo22() { return ( coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz )(Fooooooooooo.Fooooooooooo); return ( coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz )?.(Fooooooooooo.Fooooooooooo); }

Output:

function foo22() { return ( coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz )(Fooooooooooo.Fooooooooooo); return (coooooooooooooooooooooooooooooooooooooooooooooooooooond ? baaaaaaaaaaaaaaaaaaaaar : baaaaaaaaaaaaaaaaaaaaaz)?.(Fooooooooooo.Fooooooooooo); }

…nto fix-8840

thorn0 · 2021-02-04T19:06:32Z

src/language-js/print/ternary.js

+  if (
+    (parent.type === "CallExpression" ||
+      parent.type === "OptionalCallExpression") &&
+    checkAncestor(parent) &&


Can't you reuse ancestorCount here instead of climbing up the path one more time in getChainRoot?

Also what about the case where parent.type === "TSNonNullExpression"?

I meant to write this under parent.type === "MemberExpression" || parent.type === "OptionalMemberExpression". But maybe you should do the chain root check here too? After all, the search for ancestor above considers calls chain elements.

I was able to get rid of the getChainRoot function by reusing the ancestorCount, thanks!

fisker · 2021-02-05T01:11:42Z

src/language-js/print/ternary.js

+    return true;
+  }
+
+  const chainRoot = path.getParentNode(ancestorCount - 1);


The rest part in this function seems just CallExpression.callee, MemberExpression.object, and TSNonNullExpression.object? Code like this should work?

if ( isChainElement(parent) && checkAncestor(chainRoot) && ( (isCallExpression() && name === "callee") || (isMemberExpression() && name === "object") || (parent.type === 'TSNonNullExpression' && name === "object") || ) )

Note: name === "object" was not checking, we should add tests for them.

fisker · 2021-02-05T07:34:29Z

@thorn0 Do you think this fixes your old issue #4976? Your comment in
https://github.com/prettier/prettier/pull/7564/files#r379190012

fisker · 2021-02-05T09:45:10Z

src/language-js/print/ternary.js

+
+  const chainRoot = path.getParentNode(ancestorCount - 1);
+  if (
+    isChainElement(parent) &&


isChainElement check is not needed anymore, following codition already ensure it.

And please tweak the conditions order a little bit. check name first and check parent first.

fisker · 2021-02-05T09:53:37Z

src/language-js/print/ternary.js

+   *     : second
+   * ) as SomeType;
+   */
+  if (parent.type === "TSAsExpression" && checkAncestor(parent)) {


Let's merge TSAsExpression and NewExpression too,

if ( ( (name === "callee" && parent.type === "NewExpression") || (name === "expression" && parent.type === "TSAsExpression") ) && checkAncestor(parent) )

Once again, name === "expression" not checked, TSAsExpression has two children, seems typeAnnotation can't be ConditionalExpression, but let's add it to be safer.

fisker · 2021-02-05T09:57:12Z

src/language-js/print/ternary.js

  const chainRoot = path.getParentNode(ancestorCount - 1);
  if (
-    isChainElement(parent) &&
    checkAncestor(chainRoot) &&
-    ((isCallExpression(parent) && name === "callee") ||
-      (isMemberExpression(parent) && name === "object") ||
-      (parent.type === "TSNonNullExpression" && name === "expression"))
+    ((name === "callee" && isCallExpression(parent)) ||
+      (name === "object" && isMemberExpression(parent)) ||
+      (name === "expression" && parent.type === "TSNonNullExpression"))
  ) {
    return true;
  }


Suggested change

const chainRoot = path.getParentNode(ancestorCount - 1);

if (

isChainElement(parent) &&

checkAncestor(chainRoot) &&

((isCallExpression(parent) && name === "callee") ||

(isMemberExpression(parent) && name === "object") ||

(parent.type === "TSNonNullExpression" && name === "expression"))

((name === "callee" && isCallExpression(parent)) ||

(name === "object" && isMemberExpression(parent)) ||

(name === "expression" && parent.type === "TSNonNullExpression"))

) {

return true;

}

if (

((name === "callee" && isCallExpression(parent)) ||

(name === "object" && isMemberExpression(parent)) ||

(name === "expression" && parent.type === "TSNonNullExpression")) &&

checkAncestor(path.getParentNode(ancestorCount - 1))

) {

return true;

}

This reduces one getParentNode() call.

thorn0 · 2021-02-05T11:35:47Z

thorn0 Do you think this fixes your old issue #4976?

@fisker For conditionals in member expressions, yes, it looks good.

sosukesuzuki · 2021-02-07T14:32:32Z

Can we merge this?

thorn0 · 2021-02-07T20:53:30Z

Oops. This inconsistency doesn't seem to be intended.

Prettier pr-10187
Playground link

--parser babel

Input:

bifornCringerMoshedPerplexSawder = fn((
  glimseGlyphsHazardNoopsTieTie === 0
    ? averredBathersBoxroomBuggyNurl
    : anodyneCondosMalateOverateRetinol
).prop);

fn((
  glimseGlyphsHazardNoopsTieTie === 0
    ? averredBathersBoxroomBuggyNurl
    : anodyneCondosMalateOverateRetinol
).prop);

Output:

bifornCringerMoshedPerplexSawder = fn(
  (
    glimseGlyphsHazardNoopsTieTie === 0
      ? averredBathersBoxroomBuggyNurl
      : anodyneCondosMalateOverateRetinol
  ).prop
);

fn(
  (glimseGlyphsHazardNoopsTieTie === 0
    ? averredBathersBoxroomBuggyNurl
    : anodyneCondosMalateOverateRetinol
  ).prop
);

sosukesuzuki · 2021-02-07T21:56:05Z

Ooops. I didn't notice, I'll create new issue for this.

sosukesuzuki added 3 commits February 1, 2021 04:10

Fix ternary expression with AsExpression

3625f6d

Add changelog

bdb1d4a

Fix changelog

20e4edd

github-actions bot mentioned this pull request Jan 31, 2021

10187 thorn0/prettier-regression-testing#5

Closed

thorn0 approved these changes Jan 31, 2021

View reviewed changes

alexander-akait approved these changes Feb 1, 2021

View reviewed changes

fisker reviewed Feb 1, 2021

View reviewed changes

changelog_unreleased/typescript/10187.md Outdated Show resolved Hide resolved

fisker reviewed Feb 1, 2021

View reviewed changes

changelog_unreleased/typescript/10187.md Outdated Show resolved Hide resolved

sosukesuzuki and others added 7 commits February 1, 2021 21:11

Update changelog_unreleased/typescript/10187.md

d314051

Co-authored-by: fisker Cheung <[email protected]>

Support CallExpression and MemberExpression

b324f3c

Refactor

af35285

new line

36c61ac

Move rightSideNodeNamesSet to out of function

8af01b5

Merge branch 'fix-8840' of https://github.com/sosukesuzuki/prettier i…

aa5d0f6

…nto fix-8840

Fix changelog

e7dee0c

thorn0 reviewed Feb 2, 2021

View reviewed changes

sosukesuzuki added 2 commits February 4, 2021 04:43

Exclude BinaryExpression

2aa2559

Add callee

76c2db4

sosukesuzuki changed the title ~~Fix formatting for ternary operators with TypeScript as.~~ Consistent indentations for conditional operators Feb 3, 2021

sosukesuzuki requested review from fisker and thorn0 February 3, 2021 19:56

sosukesuzuki added 3 commits February 4, 2021 05:34

Pass tests

7684b35

Support TSNonNullExpression

206303f

Pass tests

11c6577

thorn0 approved these changes Feb 3, 2021

View reviewed changes

fisker reviewed Feb 4, 2021

View reviewed changes

More strictly

81c8dfd

sosukesuzuki added 2 commits February 4, 2021 21:59

Extract utils for chain

52461f2

Merge branch 'fix-8840' of https://github.com/sosukesuzuki/prettier i…

e9793d4

…nto fix-8840

thorn0 reviewed Feb 4, 2021

View reviewed changes

sosukesuzuki added 5 commits February 5, 2021 06:42

Reuse ancestorCount

fe7e51a

Refactor

fc071f2

Add tests for call expression chain

d77d35a

Support TSNonNullExpression

8679231

Remove BinaryExpression checking

b2bc068

fisker reviewed Feb 5, 2021

View reviewed changes

fisker mentioned this pull request Feb 5, 2021

Indent long ConditionalExpression in MemberExpression #7564

Closed

4 tasks

sosukesuzuki added 3 commits February 5, 2021 17:36

Merge branch 'main' into fix-8840

f00f374

Refactor

23b880e

Add tests for "name === object"

4a688cc

fisker reviewed Feb 5, 2021

View reviewed changes

Address reviews

e5a3f6d

fisker reviewed Feb 5, 2021

View reviewed changes

sosukesuzuki added 2 commits February 5, 2021 19:56

Address review

37957f7

Refactor

639b48e

fisker approved these changes Feb 5, 2021

View reviewed changes

Add tests for #prettier#4976

8e0003b

thorn0 merged commit cc1f054 into prettier:main Feb 7, 2021

sosukesuzuki deleted the fix-8840 branch February 7, 2021 15:37

sosukesuzuki mentioned this pull request Feb 7, 2021

Inconsistent indentation for ternary in function call #10265

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2022

Uh oh!

Conversation

sosukesuzuki commented Jan 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

thorn0 commented Feb 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 commented Feb 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisker Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thorn0 commented Feb 5, 2021

Uh oh!

sosukesuzuki commented Feb 7, 2021

Uh oh!

thorn0 commented Feb 7, 2021

Uh oh!

sosukesuzuki commented Feb 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sosukesuzuki commented Jan 31, 2021 •

edited

Loading

fisker Feb 4, 2021 •

edited

Loading

fisker Feb 5, 2021 •

edited

Loading

fisker commented Feb 5, 2021 •

edited

Loading

fisker Feb 5, 2021 •

edited

Loading