Break nested ternaries by duailibe · Pull Request #4120 · prettier/prettier

duailibe · 2018-03-08T14:32:46Z

We'll reuse the same logic from JSX mode to only group at the outermost ternary, forcing all ternaries in a chain to break if any of them break.

Closes #3018

lydell · 2018-03-08T17:39:01Z

I think I like this change. Perhaps we should try it out on some real project first. I can do that.

duailibe · 2018-03-08T17:43:31Z

Yeah, I was waiting already on some more approvals (for consensus) and will try on some codebases at work and some OS projects.

suchipi · 2018-03-08T18:00:00Z

@vjeux do you have an opinion? Are you concerned about impact on the facebook codebase?

vjeux · 2018-03-08T18:14:38Z

I thought this was the current behavior. I definitely implemented this feature in the past, I guess that was for a test and I didn't actually push it. I'm onboard with it. Would be great to run it on some real code first though as mentioned in the comments to see the real impact it has.

lydell · 2018-03-08T18:16:10Z

Here are the diffs I got on 51K source lines of JS:

     const rangeStartIndex =
       $rangeStart.length > 0
         ? $allCells.index($rangeStart)
-        : date < this.startDate ? $allCells.length : 0;
+        : date < this.startDate
+          ? $allCells.length
+          : 0;

           >
             {submitState.state === constants.SUBMIT_STATES.SUBMITTING
               ? 'Saving…'
-              : dirty ? 'Save your changes' : '(Nothing to save)'}
+              : dirty
+                ? 'Save your changes'
+                : '(Nothing to save)'}
           </button>

 function sortRegions(selectedRegionId, regionA, regionB) {
   return regionA.id === selectedRegionId
     ? 1
-    : regionB.id === selectedRegionId ? -1 : 0;
+    : regionB.id === selectedRegionId
+      ? -1
+      : 0;
 }

   const sign = line.y
-    ? point.x < targetPoint.x ? 1 : -1
-    : point.y < targetPoint.y ? 1 : -1;
+    ? point.x < targetPoint.x
+      ? 1
+      : -1
+    : point.y < targetPoint.y
+      ? 1
+      : -1;
   return movePointAlongLine(line, point, distance * sign);
 }

   const previewPositionId =
     selectedPositionIds.length > 0
       ? selectedPositionIds[0]
-      : selectableIds.length > 0 ? selectableIds[0] : null;
+      : selectableIds.length > 0
+        ? selectableIds[0]
+        : null;

               onClick={
                 isUnselectedAndEnabled
                   ? this.onUnselectedControlsClick.bind(this)
-                  : enabled ? null : onDisabledClick
+                  : enabled
+                    ? null
+                    : onDisabledClick
               }
             >

     const widthAdjustment = mainConstants.isInternetExplorer
       ? 1
-      : mainConstants.isEdge ? 0.5 : 0;
+      : mainConstants.isEdge
+        ? 0.5
+        : 0;

             textAnchor={
               date.getTime() === xStart.getTime()
                 ? "start"
-                : date.getTime() === xEnd.getTime() ? "end" : "middle"
+                : date.getTime() === xEnd.getTime()
+                  ? "end"
+                  : "middle"
             }

   const finalWords =
     bestWords.length > 1
       ? bestWords
-      : preferredWords.length > 0 ? preferredWords : words;
+      : preferredWords.length > 0
+        ? preferredWords
+        : words;

                   subText={
                     latestRound
                       ? maybeDate(latestUser.added_on)
-                      : shimmer ? "" : null
+                      : shimmer
+                        ? ""
+                        : null
                   }

                 >
                   {selectedAccount.type === "existing"
-                    ? success ? "Account linked" : "Link existing Account"
+                    ? success
+                      ? "Account linked"
+                      : "Link existing Account"
                     : success
                       ? "Account created and linked"
                       : "Create and link Account"}

   compareByNumber(
     ({ dateCreated, closeDate }) =>
       dateCreated != null && closeDate != null
-        ? closeDate === ONGOING_ENTRY_DATE ? 0 : 1
+        ? closeDate === ONGOING_ENTRY_DATE
+          ? 0
+          : 1
         : 2,
   ),

suchipi · 2018-03-08T18:25:46Z

The only one that I think looks better without the change is:

   const sign = line.y
-    ? point.x < targetPoint.x ? 1 : -1
-    : point.y < targetPoint.y ? 1 : -1;
+    ? point.x < targetPoint.x
+      ? 1
+      : -1
+    : point.y < targetPoint.y
+      ? 1
+      : -1;
   return movePointAlongLine(line, point, distance * sign);
 }

But I think all the other diffs are an improvement, so it's fine.

lydell · 2018-03-08T19:37:13Z

I agree.

duailibe · 2018-04-04T17:59:46Z

I finally ran on some repos, the most relevant changes I found on the react repo (there were more, only showing the ones that are "different"):

diff --git a/packages/react-dom/src/events/SyntheticWheelEvent.js b/packages/react-dom/src/events/SyntheticWheelEvent.js
index b4c49f4..16435fe 100644
--- a/packages/react-dom/src/events/SyntheticWheelEvent.js
+++ b/packages/react-dom/src/events/SyntheticWheelEvent.js
@@ -16,7 +16,9 @@ const SyntheticWheelEvent = SyntheticMouseEvent.extend({
     return 'deltaX' in event
       ? event.deltaX
       : // Fallback to `wheelDeltaX` for Webkit and normalize (right is positive).
-        'wheelDeltaX' in event ? -event.wheelDeltaX : 0;
+        'wheelDeltaX' in event
+        ? -event.wheelDeltaX
+        : 0;
   },
   deltaY(event) {
     return 'deltaY' in event
@@ -25,7 +27,9 @@ const SyntheticWheelEvent = SyntheticMouseEvent.extend({
         'wheelDeltaY' in event
         ? -event.wheelDeltaY
         : // Fallback to `wheelDelta` for IE<9 and normalize (down is positive).
-          'wheelDelta' in event ? -event.wheelDelta : 0;
+          'wheelDelta' in event
+          ? -event.wheelDelta
+          : 0;
   },
   deltaZ: null,
 
diff --git a/packages/shared/describeComponentFrame.js b/packages/shared/describeComponentFrame.js
index 3494c31..5c619e6 100644
--- a/packages/shared/describeComponentFrame.js
+++ b/packages/shared/describeComponentFrame.js
@@ -21,6 +21,8 @@ export default function(
         ':' +
         source.lineNumber +
         ')'
-      : ownerName ? ' (created by ' + ownerName + ')' : '')
+      : ownerName
+        ? ' (created by ' + ownerName + ')'
+        : '')
   );
 }

duailibe · 2018-04-04T18:06:47Z

Those are basically this change:

Prettier pr-4120
Playground link

Input:

return (
  (a || b) &&
  (cond 
    ? alternate 
    : //comment
      consequent ? is : nested)
);

Output:

return (
  (a || b) &&
  (cond
    ? alternate
    : //comment
      consequent
      ? is
      : nested)
);

Thoughts?

suchipi · 2018-04-05T00:50:12Z

I'm calling it

vjeux · 2018-04-05T00:50:50Z

Boom!

Break nested ternaries

992cb36

lipis approved these changes Mar 8, 2018

View reviewed changes

j-f1 approved these changes Mar 8, 2018

View reviewed changes

suchipi approved these changes Mar 8, 2018

View reviewed changes

suchipi merged commit e17bb5e into prettier:master Apr 5, 2018

duailibe deleted the break-nested-ternaries branch April 5, 2018 12:57

vjeux mentioned this pull request Apr 9, 2018

1.12.0 release #4231

Closed

renovate Bot mentioned this pull request Apr 12, 2018

Update prettier to 1.12.1 sbardian/DebtTrackerAPI#28

Merged

lock Bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 5, 2018

lock Bot locked as resolved and limited conversation to collaborators Jul 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Break nested ternaries#4120

Break nested ternaries#4120
suchipi merged 1 commit intoprettier:masterfrom
duailibe:break-nested-ternaries

duailibe commented Mar 8, 2018

Uh oh!

lydell commented Mar 8, 2018

Uh oh!

duailibe commented Mar 8, 2018

Uh oh!

suchipi commented Mar 8, 2018

Uh oh!

vjeux commented Mar 8, 2018

Uh oh!

lydell commented Mar 8, 2018 •

edited

Loading

Uh oh!

suchipi commented Mar 8, 2018

Uh oh!

lydell commented Mar 8, 2018

Uh oh!

duailibe commented Apr 4, 2018 •

edited

Loading

Uh oh!

duailibe commented Apr 4, 2018 •

edited

Loading

Uh oh!

suchipi commented Apr 5, 2018

Uh oh!

vjeux commented Apr 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

duailibe commented Mar 8, 2018

Uh oh!

lydell commented Mar 8, 2018

Uh oh!

duailibe commented Mar 8, 2018

Uh oh!

suchipi commented Mar 8, 2018

Uh oh!

vjeux commented Mar 8, 2018

Uh oh!

lydell commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suchipi commented Mar 8, 2018

Uh oh!

lydell commented Mar 8, 2018

Uh oh!

duailibe commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duailibe commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suchipi commented Apr 5, 2018

Uh oh!

vjeux commented Apr 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lydell commented Mar 8, 2018 •

edited

Loading

duailibe commented Apr 4, 2018 •

edited

Loading

duailibe commented Apr 4, 2018 •

edited

Loading