JavaScript: Prefer a trailing comment when comment on between test and consequent on ternary #8554

sosukesuzuki · 2020-06-14T05:27:59Z

Fixes #2076. this is re-implementation of #6418.

The below case is out of this PR scope, I'll work on it after this PR is merged(#8709):
Prettier 2.0.5
Playground link

--parser babel

Input:

test
  ? first
  // comment
  : second

Output:

test
  ? first
  : // comment
    second;

I’ve added tests to confirm my change works.
(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✨

fisker · 2020-06-21T15:57:34Z

src/language-js/comments.js

+  for (let i = start; i < end; ++i) {
+    if (text.charAt(i) === "?") {


text.indexOf('?', start)

fisker · 2020-06-21T16:03:07Z

src/language-js/comments.js

+    if (text.charAt(i) === "?") {
+      let hasQuestion = true;
+      // Ignore "?" in comments
+      for (const comment of comments) {


Can we use

prettier/src/common/util.js

Line 243 in 0213706

function getNextNonSpaceNonCommentCharacterIndexWithStartIndex(text, idx) {

?

BTW: we really should use token for this kind of stuff #8529

I'll consider it.

BTW: we really should use token for this kind of stuff #8529

Using token sounds good!

I fixed with using getNextNonSpaceNonCommentCharacterIndexWithStartIndex for now. We should replace it with using token in future.

fisker · 2020-06-21T16:21:05Z

Prettier pr-8554
Playground link

--parser babel

Input:

test // comment ?
? 
// comment
foo :  bar;

test // comment 
? 
// comment
foo :  bar;

Output:

test // comment ?
  // comment
  ? foo
  : bar;

test // comment
  ? // comment
    foo
  : bar;

fisker · 2020-06-26T16:19:08Z

I guess the token you found is )

Prettier pr-8554
Playground link

--parser babel

Input:

(await test) 
// comment
?
foo :  bar;

(test) 
// comment
?
foo :  bar;

 test
// comment
?
foo :  bar;

Output:

(await test)
  ? // comment
    foo
  : bar;

test
  ? // comment
    foo
  : bar;

test
  // comment
  ? foo
  : bar;

fisker · 2020-06-26T19:19:29Z

Blank line matters?

Prettier pr-8554
Playground link

--parser babel

Input:

test ?
// comment

foo:  bar;

test ?
// comment
foo :  bar;

Output:

test
  ? // comment

    foo
  : bar;

test
  ? // comment
    foo
  : bar;

fisker · 2020-06-26T19:21:53Z

I think you need keep looking for ?

Prettier pr-8554
Playground link

--parser babel

Input:

(test) ?
// comment
foo :  bar;

test ?
// comment
foo :  bar;

Output:

test
  // comment
  ? foo
  : bar;

test
  ? // comment
    foo
  : bar;

sosukesuzuki · 2020-06-26T20:05:53Z

@fisker

Blank line matters?

I'm not sure but maybe yes. The behavior also happens on current Prettier and other operator (e.g. BinaryExpression)

Prettier 2.0.5
Playground link

--parser babel

Input:

test ?
// comment

foo:  bar;

test ?
// comment
foo :  bar;

a +
  // comment
  
  a;

a +
  // comment
  a;

Output:

test
  ? // comment

    foo
  : bar;

test
  ? // comment
    foo
  : bar;

a +
  // comment

  a;

a +
  // comment
  a;

sosukesuzuki · 2020-06-26T20:08:31Z

I think you need keep looking for ?

I tried to fix by 776f858 with recursion.

fisker · 2020-06-27T08:29:42Z

Why not a loop

lydell · 2020-06-28T12:28:16Z

I might misunderstand #2076, but isn’t the issue about not having trailing comments? I.e. this should stay as-is:

this.chromeInstances = CONSTANTS.paralleliseChrome
  // I am running in paralel, add a chromeLauncher.launch() command for each required
  // instance of Chrome to run in paralell
  ? await Promise.all([
      chromeLauncher.launch(chromeLauncherOptions),
      chromeLauncher.launch(chromeLauncherOptions),
      chromeLauncher.launch(chromeLauncherOptions)
    ])
  // Every test run will be in the same instance of Chrome
  : await chromeLauncher.launch(chromeLauncherOptions);

fisker

Looks good, but I don't like adding options.printer.shouldIndentComment and I don't have a better idea 😄

sosukesuzuki · 2020-06-28T14:28:47Z

@lydell

I might misunderstand #2076, but isn’t the issue about not having trailing comments? I.e. this should stay as-is:

Sorry I cannot understand you said...

(This PR partially fixes the example you showed. I'll also fix the problem about comments before : after this PR is merged.)

Prettier pr-8554
Playground link

--parser babel

Input:

this.chromeInstances = CONSTANTS.paralleliseChrome
  // I am running in paralel, add a chromeLauncher.launch() command for each required
  // instance of Chrome to run in paralell
  ? await Promise.all([
      chromeLauncher.launch(chromeLauncherOptions),
      chromeLauncher.launch(chromeLauncherOptions),
      chromeLauncher.launch(chromeLauncherOptions)
    ])
  // Every test run will be in the same instance of Chrome
  : await chromeLauncher.launch(chromeLauncherOptions);

Output:

this.chromeInstances = CONSTANTS.paralleliseChrome
  // I am running in paralel, add a chromeLauncher.launch() command for each required
  // instance of Chrome to run in paralell
  ? await Promise.all([
      chromeLauncher.launch(chromeLauncherOptions),
      chromeLauncher.launch(chromeLauncherOptions),
      chromeLauncher.launch(chromeLauncherOptions),
    ])
  : // Every test run will be in the same instance of Chrome
    await chromeLauncher.launch(chromeLauncherOptions);

src/language-js/comments.js

fisker · 2020-06-29T07:53:10Z

@sosukesuzuki Is this expected? I tried your version of that loop, it's the same.

Prettier pr-8554
Playground link

--parser babel

Input:

test ?// comment
  foo
  : bar;

test 
?// comment
  foo
  : bar;

Output:

test // comment
  ? foo
  : bar;

test
  ? // comment
    foo
  : bar;

sosukesuzuki · 2020-06-29T08:07:39Z

Thank you for your fix.
I think second example is expected, but first example result is expected as:

test
  ? // comment
    foo
  : bar;

I'll look into this.

sosukesuzuki · 2020-07-05T15:27:51Z

@fisker Stable Prettier also formats like you showed. I want to keep PRs small as possible as so can I fix this problem in other PR?
Prettier 2.0.5
Playground link

--parser babel

Input:

test ?// comment
  foo
  : bar;

Output:

test // comment
  ? foo
  : bar;

sosukesuzuki · 2020-07-06T08:15:38Z

@thorn0 @evilebottnawi Could you review?

alexander-akait

Looks very good

thorn0 · 2020-07-10T09:33:16Z

@sosukesuzuki I'm really sorry for being slow with reviewing. I'll get to this ASAP.

thorn0 · 2020-07-13T14:11:31Z

My main concerns are prettier-ignore comments and that shouldIndentComment and shouldDedentComment don't look reusable/universal enough to be added to the printer API. As for prettier-ignore, here's what I mean:

Prettier pr-8554
Playground link

--parser babel

Input:

a=foo&&       bar 
  // prettier-ignore
  ? baz     +quux : 20;

Output:

a =
  foo&&       bar
    // prettier-ignore
    ? baz + quux
    : 20;

We had a similar problem (#7798) in union types where comments are attached as trailing too, for the same reasons. Can we probably reuse the way comments are printed in union types (willPrintOwnComments and printComments)?

sosukesuzuki added 12 commits June 12, 2020 13:51

Handle ternary comments

b2d58f2

Add indent

90ba3e5

Add comment

e88bf46

Update tests

3d01c85

Update tests

dac148e

Support dedent

4295766

Refactor

d4e7ad7

Fix

cb097e5

Support comment in between "?" and second op

1d77560

Refactor

8afc04d

Refactor

1feddd4

Add changelog

57da1e1

sosukesuzuki marked this pull request as draft June 14, 2020 05:42

This was referenced Jun 14, 2020

TypeScript: ugly formatting for comment after "?" in conditional types #8555

Closed

JavaScript: incorrecct formatting for multiline block comments after ternary "?" #8556

Closed

sosukesuzuki added 3 commits June 14, 2020 18:42

Rename

74fb69d

Merge branch 'master' into 2076

ebe3a81

Move functions

2ed9474

sosukesuzuki marked this pull request as ready for review June 21, 2020 10:17

sosukesuzuki requested review from thorn0 and fisker June 21, 2020 15:23

fisker reviewed Jun 21, 2020

View reviewed changes

Refactor with using getNextNonSpaceNonCommentCharacterWithStartIndex

f2bbf89

sosukesuzuki added 2 commits June 27, 2020 03:10

Fix to avoid paren

62290e6

Fix

776f858

With for loop

a5b4a08

fisker approved these changes Jun 28, 2020

View reviewed changes

sosukesuzuki requested a review from alexander-akait June 28, 2020 14:38

fisker reviewed Jun 29, 2020

View reviewed changes

src/language-js/comments.js Outdated Show resolved Hide resolved

Try to make loop faster

aa4cb49

sosukesuzuki mentioned this pull request Jul 5, 2020

JavaScript: Incorrect formatting for conditional expression and end of line comment after ? #8708

Open

fisker approved these changes Jul 5, 2020

View reviewed changes

alexander-akait approved these changes Jul 6, 2020

View reviewed changes

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

hwalinga mentioned this pull request Apr 20, 2021

Issue with comments & nested ternaries #10051

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JavaScript: Prefer a trailing comment when comment on between test and consequent on ternary #8554

JavaScript: Prefer a trailing comment when comment on between test and consequent on ternary #8554

sosukesuzuki commented Jun 14, 2020 •

edited

fisker Jun 21, 2020 •

edited

fisker Jun 21, 2020

sosukesuzuki Jun 21, 2020

sosukesuzuki Jun 25, 2020

fisker commented Jun 21, 2020

fisker commented Jun 26, 2020

fisker commented Jun 26, 2020

fisker commented Jun 26, 2020

sosukesuzuki commented Jun 26, 2020

sosukesuzuki commented Jun 26, 2020

fisker commented Jun 27, 2020

lydell commented Jun 28, 2020

fisker left a comment

sosukesuzuki commented Jun 28, 2020

fisker commented Jun 29, 2020 •

edited

sosukesuzuki commented Jun 29, 2020

sosukesuzuki commented Jul 5, 2020

sosukesuzuki commented Jul 6, 2020

alexander-akait left a comment

thorn0 commented Jul 10, 2020

thorn0 commented Jul 13, 2020

		for (let i = start; i < end; ++i) {
		if (text.charAt(i) === "?") {

JavaScript: Prefer a trailing comment when comment on between test and consequent on ternary #8554

Are you sure you want to change the base?

JavaScript: Prefer a trailing comment when comment on between test and consequent on ternary #8554

Conversation

sosukesuzuki commented Jun 14, 2020 • edited

fisker Jun 21, 2020 • edited

Choose a reason for hiding this comment

fisker Jun 21, 2020

Choose a reason for hiding this comment

sosukesuzuki Jun 21, 2020

Choose a reason for hiding this comment

sosukesuzuki Jun 25, 2020

Choose a reason for hiding this comment

fisker commented Jun 21, 2020

fisker commented Jun 26, 2020

fisker commented Jun 26, 2020

fisker commented Jun 26, 2020

sosukesuzuki commented Jun 26, 2020

sosukesuzuki commented Jun 26, 2020

fisker commented Jun 27, 2020

lydell commented Jun 28, 2020

fisker left a comment

Choose a reason for hiding this comment

sosukesuzuki commented Jun 28, 2020

fisker commented Jun 29, 2020 • edited

sosukesuzuki commented Jun 29, 2020

sosukesuzuki commented Jul 5, 2020

sosukesuzuki commented Jul 6, 2020

alexander-akait left a comment

Choose a reason for hiding this comment

thorn0 commented Jul 10, 2020

thorn0 commented Jul 13, 2020

sosukesuzuki commented Jun 14, 2020 •

edited

fisker Jun 21, 2020 •

edited

fisker commented Jun 29, 2020 •

edited