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

fix(12077) line breaking with unary operators in conditional #12087

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

islandryu
Copy link

@islandryu islandryu commented Jan 15, 2022

Description

fix #12077

Unary operators within Conditional operators will break lines.
In other words, it does the same thing as the binary operator.

playground

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (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

@islandryu islandryu changed the title fix line breaking with unary operators in conditional fix(12077) line breaking with unary operators in conditional Jan 15, 2022
@islandryu islandryu marked this pull request as ready for review January 15, 2022 12:54
@islandryu
Copy link
Author

I forgot to format the markdown, so I ran it.

@fisker
Copy link
Member

fisker commented Jan 18, 2022

There are lots of other UnaryOperators, if you mean to include them all, add more tests.

islandryu and others added 2 commits January 19, 2022 11:36
Co-authored-by: fisker Cheung <lionkay@gmail.com>
Co-authored-by: fisker Cheung <lionkay@gmail.com>
@islandryu
Copy link
Author

@fisker
Thanks for the confirmation.
It's a little late now, but regardless of whether it's binary or unary, the line break position should be consistent, right?

// printWidth: 25
// input
const c = {
  prop1: foo == null ? bar : baz,
  prop2: !fooooooooo ? bar : baz,
// Should Identifier also have a new line?
  prop3: foooooooooo ? bar : baz,
};

// output
const c = {
  prop1:
    foo == null
      ? bar
      : baz,
  prop2:
    !fooooooooo
      ? bar
      : baz,
// Should Identifier also have a new line?
  prop3:
    foooooooooo
      ? bar
      : baz,
};

@islandryu islandryu requested a review from fisker January 19, 2022 11:01
@fisker
Copy link
Member

fisker commented Jan 19, 2022

Should Identifier also have a new line?

Maybe we can try the opposite, instead of break before !fooooooooo, but remove new line before foo == null? There are too many cases to handle.

Prettier pr-12087
Playground link

--parser babel
--print-width 25

Input:

const x = {
  prop1: !fooooooooo ? bar : baz,
  prop1: fooooooooo ? bar : baz,
  prop1: foooo.oooo ? bar : baz,
  prop1: foooo.oo() ? bar : baz,
};

Output:

const x = {
  prop1:
    !fooooooooo
      ? bar
      : baz,
  prop1: fooooooooo
    ? bar
    : baz,
  prop1: foooo.oooo
    ? bar
    : baz,
  prop1: foooo.oo()
    ? bar
    : baz,
};

@islandryu
Copy link
Author

@fisker
If we remove the new line before foo == null, we get this.

// printWidth: 18
// input
const x = {
  prop1: foo === null ? bar : baz,
  prop1: !fooooooooo ? bar : baz,
}

// output
const x = {
  prop1: foo ===
  null
    ? bar
    : baz,
  prop1:
    !fooooooooo
      ? bar
      : baz
}

Therefore, we changed the pattern so that all patterns have a new line.
Is there a problem if I change it so that all patterns have a new line?

@fisker
Copy link
Member

fisker commented Jan 20, 2022

Is there a problem if I change it so that all patterns have a new line?

I'm not familiar with this part, I don't know, sorry.

@fisker
Copy link
Member

fisker commented Jan 20, 2022

The output looks good, I'll check later.

@fisker
Copy link
Member

fisker commented Jan 20, 2022

I ran this change over some codebase, some case doesn't look good. mostly the variable declaration

-          const unwrappedInit = hasUnaryPrefix(init, '-')
-            ? init.argument
-            : init;
+          const unwrappedInit =
+            hasUnaryPrefix(init, '-') ? init.argument : init;

Full diff prettier/prettier-regression-testing#130

@islandryu
Copy link
Author

If such a case exists, I guess I'll have to delete the new line before binaryExpression.

@islandryu
Copy link
Author

Remove newline before binaryExpression. Existing tests have been affected.
It is not possible to make the newline constant for ternary operators without affecting existing tests.

@fisker
Copy link
Member

fisker commented Jan 26, 2022

It's fine to change the output.

Comment on lines 2 to 3
run_spec(__dirname, ["babel", "flow", "typescript"], { printWidth: 18 });
run_spec(__dirname, ["babel", "flow", "typescript"], { printWidth: 25 });
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the new tests longer to cross 80 width, so we can remove the printWidth? Or you can put new tests into a new directory, the snap became very big, hard to review.

@fisker
Copy link
Member

fisker commented Jan 26, 2022

I'm running this over other codebases, let's see how this effect them prettier/prettier-regression-testing#130 (comment)

@fisker
Copy link
Member

fisker commented Jan 26, 2022

The new output looks better to me. And changes in #12087 #10158 seems not effected 👍. Anyway, we need more review.

@islandryu
Copy link
Author

I put the tests for printWidth: 18 and printWidth: 25 in a separate folder.

@islandryu islandryu requested a review from fisker January 30, 2022 14:47
@thorn0 thorn0 self-requested a review August 29, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent formatting for object properties with ternaries
2 participants