Skip to content
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

[WIP] prettierx refactor!: fix & update balanced ternary formatting support - DRAFT WIP #620

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from
Draft

[WIP] prettierx refactor!: fix & update balanced ternary formatting support - DRAFT WIP #620

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ea4ec7f
prettierx: fix balanced ternary formatting, with a simpler solultion
Mar 8, 2021
a141e49
update existing docs with reference to brodybits/prettierx issue 552
Jun 23, 2021
c81099e
Merge branch 'fix-balanced-ternary-formatting' of https://github.com/…
Jun 23, 2021
8fef2ec
update test snapshots in tests/ternaries/with-balanced-formatting (...)
Jun 23, 2021
9c88a28
add note: resolved #468 (brodybits/prettierx issue #468)
Jun 23, 2021
d00e6b5
prettierx chore: set version to 0.19.0-dev
Jun 24, 2021
a7e7612
Merge branch 'dev-update-wip' of https://github.com/brodybits/prettie…
Jun 24, 2021
03a79c6
revert simpler solution & test updates for now, at least, with some c…
Jun 24, 2021
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ prettierx <options> <file(s)>
## Additional prettierX options

- `--align-object-properties` (`alignObjectProperties: true`): Align colons in multiline object literals (not applied with any of the JSON parsers).
- `--offset-ternary-expressions` (`offsetTernaryExpressions: true`): Indent and align ternary expression branches more consistently with "Standard JS" (similar to the corresponding eslint option).
- `--offset-ternary-expressions` (`offsetTernaryExpressions: true`): Indent and align ternary expression branches more consistently with "Standard JS" (similar to the corresponding eslint option). NOT recommended together with `--use-tabs` due to KNOWN ISSUE: [`brodybits/prettierx#552`](https://github.com/brodybits/prettierx/issues/552)
- `--space-before-function-paren` (`spaceBeforeFunctionParen: true`): Put a space before function parenthesis in all declarations (similar to the corresponding eslint option). (Default is to put a space before function parenthesis for untyped anonymous functions only.)
- `--generator-star-spacing` (`generatorStarSpacing: true`): Put spaces around the star (`*`) in generator functions (before and after - similar to the corresponding eslint option). (Default is after only.)
- `--yield-star-spacing` (`yieldStarSpacing: true`): Put spaces around the star (`*`) in `yield*` expressions (before and after - similar to the corresponding eslint option). (Default is after only.)
Expand Down
2 changes: 1 addition & 1 deletion docs/options.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ Put spaces between computed property brackets (similar to the corresponding esli

## Offset ternary expressions

Indent and align ternary expression branches more consistently with "Standard JS" (similar to the corresponding eslint option).
Indent and align ternary expression branches more consistently with "Standard JS" (similar to the corresponding eslint option). NOT recommended together with `--use-tabs` due to KNOWN ISSUE: [`brodybits/prettierx#552`](https://github.com/brodybits/prettierx/issues/552)

| Default | CLI Override | API Override |
| ------- | ------------------------------ | ---------------------------------- |
Expand Down
9 changes: 5 additions & 4 deletions src/language-js/printer-estree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ function printTernaryOperator(path, options, print, operatorOptions) {
? ifBreak("", concat(["(", parenSpace]))
: "",
// [prettierx] offsetTernaryExpressions option support:
!options.offsetTernaryExpressions
true // !options.offsetTernaryExpressions
? align(2, path.call(print, operatorOptions.consequentNodePropertyName))
: path.call(print, operatorOptions.consequentNodePropertyName),
// [prettierx] spaceInParens option support (...)
Expand All @@ -443,7 +443,8 @@ function printTernaryOperator(path, options, print, operatorOptions) {
line,
": ",
// [prettierx] offsetTernaryExpressions option support:
options.offsetTernaryExpressions ||
// options.offsetTernaryExpressions ||
!options.offsetTernaryExpressions &&
alternateNode.type === operatorOptions.conditionalNodeType
? path.call(print, operatorOptions.alternateNodePropertyName)
: align(2, path.call(print, operatorOptions.alternateNodePropertyName)),
Expand All @@ -454,7 +455,7 @@ function printTernaryOperator(path, options, print, operatorOptions) {
parent[operatorOptions.alternateNodePropertyName] === node ||
isParentTest
? part
: options.useTabs || options.offsetTernaryExpressions // [prettierx] offsetTernaryExpressions option support (...)
: options.useTabs //|| options.offsetTernaryExpressions // [prettierx] offsetTernaryExpressions option support (...)
? dedent(indent(part))
: align(Math.max(0, options.tabWidth - 2), part)
);
Expand All @@ -463,7 +464,7 @@ function printTernaryOperator(path, options, print, operatorOptions) {
// Indent the whole ternary if offsetTernaryExpressions is enabled
// (like ESLint).
if (options.offsetTernaryExpressions) {
forceNoIndent = false;
// forceNoIndent = false;
}
}

Expand Down
60 changes: 30 additions & 30 deletions tests/ternary-object-expressions/with-balanced-formatting/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ let isSpace = false;

const dress = isSpace
? {
spaceSuit: 3,
oxygenCylinders: 6
}
spaceSuit: 3,
oxygenCylinders: 6
}
: {
shirts: 3,
paints: 3
};
shirts: 3,
paints: 3
};

================================================================================
`;
Expand Down Expand Up @@ -60,13 +60,13 @@ let isSpace = false;

const dress = isSpace
? {
spaceSuit: 3,
oxygenCylinders: 6
}
spaceSuit: 3,
oxygenCylinders: 6
}
: {
shirts: 3,
paints: 3
};
shirts: 3,
paints: 3
};
Copy link
Owner Author

@brodycj brodycj Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

From a closer look at some snapshot changes at this point with update from PR #492, with tabWidth: 4:

@@ -60,13 +60,13 @@ let isSpace = false;
 
 const dress = isSpace
     ? {
-        spaceSuit: 3,
-        oxygenCylinders: 6
-    }
+          spaceSuit: 3,
+          oxygenCylinders: 6
+      }
     : {
-        shirts: 3,
-        paints: 3
-    };
+          shirts: 3,
+          paints: 3
+      };
 
 ================================================================================
 `;

The alignment of the inner object members in this diff felt little off to me with my magnified terminal font. I think this is because the inner object members have an indentation of 10 spaces, while most of the other lines have an indentation of 4 or 6 spaces.

I am now thinking that it would be best to use an option to remove the ternary indenting, like "--no-ternary-inner-indent" as discussed in #585 in case of tab width > 2 spaces or --use-tabs option.

P.S. I think this comment is a manifestation of this issue: prettier/prettier#4203


================================================================================
`;
Expand Down Expand Up @@ -96,13 +96,13 @@ let isSpace = false;

const dress = isSpace
? {
spaceSuit: 3,
oxygenCylinders: 6
}
spaceSuit: 3,
oxygenCylinders: 6
}
: {
shirts: 3,
paints: 3
};
shirts: 3,
paints: 3
};
Copy link
Owner Author

@brodycj brodycj Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Here is the diff from my terminal with display tab width of 8:

@@ -96,13 +96,13 @@ let isSpace = false;
 
 const dress = isSpace
        ? {
-               spaceSuit: 3,
-               oxygenCylinders: 6
-       }
+                       spaceSuit: 3,
+                       oxygenCylinders: 6
+         }
        : {
-               shirts: 3,
-               paints: 3
-       };
+                       shirts: 3,
+                       paints: 3
+         };
 
 ================================================================================
 `;

I think a workaround like using a "--no-ternary-inner-indent" option as discussed in #585 is clearly needed when using the balanced ternary formatting option together with --use-tabs.

P.S. I think this observation is a manifestation of issue #552, as I reported in this comment: prettier/prettier#4203 (comment)


================================================================================
`;
Expand Down Expand Up @@ -133,13 +133,13 @@ let isSpace = false;

const dress = isSpace
? {
spaceSuit: 3,
oxygenCylinders: 6
}
spaceSuit: 3,
oxygenCylinders: 6
}
: {
shirts: 3,
paints: 3
};
shirts: 3,
paints: 3
};

================================================================================
`;
Expand Down Expand Up @@ -224,8 +224,8 @@ function test1() {
? Promise(1)
: Promise(2)
: condition3
? Promise(3)
: Promise(4);
? Promise(3)
: Promise(4);
}

================================================================================
Expand Down Expand Up @@ -268,8 +268,8 @@ function test1() {
? Promise(1)
: Promise(2)
: condition3
? Promise(3)
: Promise(4);
? Promise(3)
: Promise(4);
}

================================================================================
Expand Down Expand Up @@ -313,8 +313,8 @@ function test1() {
? Promise(1)
: Promise(2)
: condition3
? Promise(3)
: Promise(4);
? Promise(3)
: Promise(4);
}

================================================================================
Expand Down