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.

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

Conversation

brodycj
Copy link
Owner

@brodycj brodycj commented Jun 23, 2021

(BREAKING CHANGE)

resolves #468

Status: DRAFT WIP

TODO:

brodycj added 5 commits March 8, 2021 13:42
in ternary formatting with objects & tabs ref:

- #552
…brodybits/prettierx into update-balanced-ternary-formatting-support

include update from:

- #492

(brodybits/prettierx PR #492)
add commit notes:

The changes from these updates close #492 (brodybits/prettierx PR #492).

add footer:

BREAKING CHANGE
@brodycj brodycj added the bug Something isn't working label Jun 23, 2021
@brodycj brodycj self-assigned this Jun 23, 2021
Comment on lines 61 to 69
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

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

Comment on lines 97 to 105
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

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)

@brodycj brodycj changed the base branch from dev to dev-update-wip June 24, 2021 00:24
brodycj added 2 commits June 23, 2021 20:34
… into update-balanced-ternary-formatting-support
…ommit notes below (...)

* Revert "prettierx: fix balanced ternary formatting, with a simpler solultion"

This reverts commit ea4ec7f.

* Revert "update test snapshots in tests/ternaries/with-balanced-formatting (...)"

This reverts commit 8fef2ec.

ADDING COMMIT NOTES:

The following statements for GitHub should not be considered valid at this point:

> The changes from these updates close [issue number] 492 (brodybits/prettierx PR [number] 492).

> resolved [sic] [issue number] 468 (brodybits/prettierx issue [number] 468)

Some more testing & consideration is needed before moving forward with a
solution in this or another PR.
@brodycj brodycj force-pushed the dev-update-wip branch 3 times, most recently from 0c6ebbc to cf25e85 Compare June 27, 2021 18:52
@brodycj brodycj changed the base branch from dev-update-wip to dev June 28, 2021 14:41
@moussaab-moulim
Copy link

hello @brodybits
just a question is this package is still working
is it suitable to be used now
i can see that it has been 3 years since last update there a lot of things that i would live to have in my project but this is scaring me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

known --offset-ternary-expressions formatting issues
2 participants