add parens around mixed division and addition or subtraction operators for readability by brad-decker · Pull Request #9026 · prettier/prettier

brad-decker · 2020-08-22T20:23:43Z

partially addresses #187 by applying the same logic of the '%' operator
to the '/' operator. One exception is where there are leading comments:

e.g

return (
  // Reason for 42
  42
) / 8 + 2;

will not be formatted to avoid mangling the output.

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/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

✨Try the playground for this PR✨

alexander-akait · 2020-08-24T09:51:03Z

tests/js/comments/__snapshots__/jsfmt.spec.js.snap

 function binaryInBinaryLeft() {
  return (
-    // Reason for 42
+    (// Reason for 42


Ooh yeah good catch, @evilebottnawi -- seems like the comment is being considered part of the parent operation. I'll see if I can fix this. If you happen to have any advice on where to look i'd appreciate it!

@evilebottnawi this is my first time contributing here and I'm trying to dig into this a bit. It doesn't seem like from the context of needs-parens that you can really specify where the parens should be added, or add a linebreak after a comment. However, running prettier a second time against the input yields the much cleaner:

function binaryInBinaryLeft() { return ( // Reason for 42 (42 * 84) + 2 ); }

So I guess there are two options one of which I'm pretty unsure of where to look for examples of how to achieve the effect:

Option 1: Skip Nodes with comments

Essentially we'd just look if either the left or right side of the node has comments and return false in needs-parens. This would allow us to care for the vast majority of mixed-operators issues. Remaining issues that end users are having can be resolved by moving the comments manually.

Option 2: Change the starting line/col of parenthesis

The problem with the output stems from the comment being a part of the expression. I tried poking around in printer-estree with parts which is having parenthesis injected into it if needsParens is true. However, I'm not sure what the desired output should be or how to achieve it.

function binaryInBinaryLeft() { return ( // Reason for 42 (42 * 84) + 2 ); }

seems like the right approach, but I'm getting a little lost on the proper way to achieve this result. If anyone reading this has pointers I'm all ears and very eager to have this PR be considered for inclusion in prettier.

I went with the first option for now.

fisker · 2020-08-31T23:35:43Z

I like the idea, but I don't think it's improving readability in many cases.

For example

((2 / 3) * 10) / 2 + 2; (from our tests)

VS

(((2 / 3) * 10) / 2) + 2;

I would prefer.

(
  ((2 / 3) * 10) / 2
) + 2;

I'm not sure what to do

brad-decker · 2020-09-01T15:10:58Z

Thanks for your response @fisker :D. I obviously have a preference for (((2 / 3) * 10) / 2) + 2 for a lot of the reasons mentioned in #187 over the original output. I'm indifferent about breaking it into multiple lines. For me adding the parenthesis to separate the division from the addition at the end of the equation is particularly helpful, and I think that how it is formatted in terms of breaking into multiple lines adds less overall value than having the parenthesis.

I would also say that there are those on either side of the no-mixed-operators line in prettier, and I think any additional changes to formatting the output to exist across multiple lines, etc, will further complicate that discussion.

It would be my personal preference to first add the parenthesis and tackle any additional formatting in a separate pull request, perhaps after getting some community feedback on the approach?

brad-decker · 2020-09-03T16:22:31Z

@lydell -- you mentioned in #187 that a PR to add parenthesis to mixed operators in this manner would have a high likelihood of being merged.

#187 (comment)

I would greatly appreciate any feedback you can provide that would help this be easier to get approval on and merge. @fisker has some feedback and I tried to respond to feedback from @evilebottnawi but I'm not sure if my approach with skipping expressions with comments interspersed is preferred. Thank you!

lydell · 2020-09-03T16:34:39Z

@brad-decker In my comment you linked to:

Not sure what to say about multiplication.

It’s division, not multiplication, that I think has a high chance of getting merged.

brad-decker · 2020-09-03T16:51:58Z

Oops, totally jumped to conclusions on the multiplication part. So, just to confirm, @lydell:

(100 / 10) + 2

is easier to merge/approve than

(100 * 10) + 2

Is the multiplication stuff just more contentious in terms of style/necessity or is there more complexities in it? As long as my comment above is correct I'll go ahead and remove multiplication from this PR and only focus on division and if there is any way to get multiplication considered I'll address it in a future PR. Thanks for your time and attention.

thorn0 · 2020-09-03T17:25:38Z

Is the multiplication stuff just more contentious in terms of style/necessity

Yes, probably. It might even be a cultural thing. For ex-USSR-ians, parens in expressions like a + (b * c) might look really bad, even confusing. They raise questions: What do these redundant things do here? Do they carry some additional information that I'm unable to see? Or is there a mistake in the expression? For Americans, on the other hand, such parens seem to be okay, even preferable. It probably comes down to how much people are exposed to math in their childhood.

brad-decker · 2020-09-03T18:17:49Z

Well, today I learned. My apologies for my uncultured assumption -- and I imagine that this ties into @fisker 's feedback regarding the breaking of parenthesis to be more legible. Sorry! I've updated this PR to only apply to division.

thorn0 · 2020-09-03T18:39:54Z

@brad-decker Sorry if the "cultural" is the wrong word. I used it only in a geographic/ethnographic sense. In the sense that people see these things differently depending on their origin. I totally didn't mean that somebody is more cultured than somebody else. In our schools, such parens are considered a mistake. Seeing people here want to have them in JS was a TIL moment for me too.

brad-decker · 2020-09-03T18:44:26Z

I assure you I am less cultured than others, took no offense to your message, and hopefully did not offend others with mine by assuming my way of looking at math is the correct one. Thanks for calling it out, @thorn0!

This commit adds parens around mixed operators in binary expressions when the expression includes mixed division and addition or subtraction operators. To avoid some issues regarding comments appearing within existing parens, this will not format binary expressions with leading line comments e.g. ```js return ( // Reason for 42 42 ) / 8 + 2; ``` will not be formatted to avoid a mess with the position of the parens and comment.

brad-decker · 2020-09-03T19:47:41Z

@fisker the switch to focusing on division doesn't actually change anything for your comment/suggestion. The output is the same. Does the reduced scope of the change help to alleviate some of the concerns or should I explore how to break this expression over multiple lines?

thorn0 · 2020-09-03T20:08:08Z

Breaking over multiple lines sounds like a separate issue. Similar to what is discussed with regard to function calls in #6921.

fisker · 2020-09-03T22:28:56Z

Prettier pr-9026
Playground link

--parser babel

Input:

a= 42 /* comment */ / 84 + 1
a= 42 / 84 /* comment */ + 1

Output:

a = 42 /* comment */ / 84 + 1;
a = (42 / 84) /* comment */ + 1;

I think we should add parens for first one too.

fisker · 2020-09-03T22:55:10Z

And I don't think we should treat % and / differently, maybe we can improve the case @evilebottnawi pointed out in seperate PR.

Prettier pr-9026
Playground link

--parser babel

Input:

return (
  // Reason for 42
  42
) / 8 + 2;

return (
  // Reason for 42
  42
) % 8 + 2;

Output:

return (
  // Reason for 42
  42 /
    8 +
  2
);

return (
  (// Reason for 42
  42 %
    8) +
  2
);

alexander-akait · 2020-09-04T11:24:27Z

tests/js/comments/__snapshots__/jsfmt.spec.js.snap

+      84 +
+    2
+  )
+}


I am fine with fixing comments in future, but it is really ugly, we should fix it here

In that case we can open another issue, fix this first

Prettier 2.1.1
Playground link

--parser babel

Input:

function foo() { return ( // Reason for 42 42 ) % 8 + 2; }

Output:

function foo() { return ( (// Reason for 42 42 % 8) + 2 ); }

I don't think we should merge this commits with a lot of future issues, instead we should solve it here

I mean fix % one first, then this.

What would the desired output look like? I tried to walk through the codebase to find the proper place to move/format lines/parens/comments and was kind of lost. I am committed to learning how to work with the prettier codebase to become an active contributor, so I'm going to give it another go. If you wouldn't mind giving me a target to shoot for in the output? @fisker @evilebottnawi

This one may won't be easy to fix. Format original one 3 times, you'll get this.

function foo() { return ( // Reason for 42 (42 % 8) + 2 ); }

@fisker yeah I had the same result, I tried looking in the parser where needsParens is called and manipulating the insertion point of the parenthesis to be after the comment, but the comment is already printed at that point. I tried copying the pattern that was used for flow type annotations where the comment object is mutated to have printed = true and then the comment is manually inserted. I thought that would do the trick but like I said it seems in that logic tree, or with leading comments, it is already printed at the point it gets to inserting the parens?

in the example here, if you move the comment in the input to be above the return statement, the output is as you'd want... This is why I was hoping to manipulate the printing of the comment to be before the inserted opening parens.

This is really a separate issue, namely #7746
I don't think it should be fixed here.

johannesjo · 2021-05-21T12:13:37Z

Is there any help needed with this?

jsg2021 · 2021-05-28T16:40:05Z

Would this address parentheses around sequence expressions?

ilfroloff · 2023-11-29T09:20:11Z

Guys, any updates for the PR? It would be great to close the initial question #187 which is almost 7 years old 😀

brad-decker force-pushed the add-parens-around-multiplicative-operations branch 2 times, most recently from 50ba3bb to 955175f Compare August 22, 2020 20:45

brad-decker marked this pull request as ready for review August 22, 2020 22:14

brody2consult mentioned this pull request Aug 23, 2020

parens around multiplicative operators - readability brody2consult/prettierx#364

Open

alexander-akait reviewed Aug 24, 2020

View reviewed changes

brad-decker force-pushed the add-parens-around-multiplicative-operations branch from 955175f to 51b5253 Compare August 31, 2020 20:31

brad-decker requested a review from alexander-akait August 31, 2020 20:33

brad-decker mentioned this pull request Sep 1, 2020

[RFC] add prettier to eslint MetaMask/metamask-extension#8595

Merged

rekmarks mentioned this pull request Sep 1, 2020

Removes parentheses in expressions with mixed operators #187

Open

brad-decker force-pushed the add-parens-around-multiplicative-operations branch from 51b5253 to 3fb6bc4 Compare September 3, 2020 17:48

brad-decker changed the title ~~add parens around multiplicative operators for readability~~ add parens around mixed division and addition or subtraction operators for readability Sep 3, 2020

brad-decker force-pushed the add-parens-around-multiplicative-operations branch from 3fb6bc4 to f91a9c9 Compare September 3, 2020 18:36

brad-decker force-pushed the add-parens-around-multiplicative-operations branch from f91a9c9 to e0a3698 Compare September 3, 2020 19:08

fisker mentioned this pull request Sep 4, 2020

Unnecessary line breaks in return #9139

Closed

alexander-akait reviewed Sep 4, 2020

View reviewed changes

Base automatically changed from master to main January 23, 2021 17:13

This was referenced Jan 27, 2021

Prettier removes desired parentheses. #10165

Closed

Unstable binary expressions format #7746

Open

++
+
+                )
+              }

Uh oh!

Conversation

brad-decker commented Aug 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Option 1: Skip Nodes with comments

Option 2: Change the starting line/col of parenthesis

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker commented Aug 31, 2020

Uh oh!

brad-decker commented Sep 1, 2020

Uh oh!

brad-decker commented Sep 3, 2020

Uh oh!

lydell commented Sep 3, 2020

Uh oh!

brad-decker commented Sep 3, 2020

Uh oh!

thorn0 commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brad-decker commented Sep 3, 2020

Uh oh!

thorn0 commented Sep 3, 2020

Uh oh!

brad-decker commented Sep 3, 2020

Uh oh!

brad-decker commented Sep 3, 2020

Uh oh!

thorn0 commented Sep 3, 2020

Uh oh!

fisker commented Sep 3, 2020

Uh oh!

fisker commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait Sep 4, 2020 • 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 Sep 4, 2020 • 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!

alexander-akait Sep 4, 2020 • 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!

fisker Sep 4, 2020 • 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!

johannesjo commented May 21, 2021

Uh oh!

jsg2021 commented May 28, 2021

Uh oh!

ilfroloff commented Nov 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

brad-decker commented Aug 22, 2020 •

edited

Loading

thorn0 commented Sep 3, 2020 •

edited

Loading

fisker commented Sep 3, 2020 •

edited

Loading

alexander-akait Sep 4, 2020 •

edited

Loading

fisker Sep 4, 2020 •

edited

Loading

alexander-akait Sep 4, 2020 •

edited

Loading

fisker Sep 4, 2020 •

edited

Loading