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

JavaScript: Keep comments position for assignment/variable #7709

Merged
merged 18 commits into from Mar 10, 2020

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Mar 4, 2020

Sorry, this is not related to 2.0


Fixes #6120

Currently, the use of almost expressions in assignments/variable-declarations containing BlockComment move the position of the comment to before =. The behavior also break the Closure Compiler type casting comments(ref :#4124 (comment)).
This PR fixes the behavior by making it consistent with array, object literal and template literal.

// Input
const foo = /* comments */
  bar;
const foo = /* comment */
  [bar];

// Current output
const foo /* comments */ = bar;
const foo =
  /* comment */
  [bar];

// This PR
const foo =
  /* comments */
  bar;
const foo =
  /* comment */
  [bar];
  • 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

@@ -69,3 +69,6 @@ const style2 =/**
const v6 = /**@type {string} */(value);
const v7 = /** @type{string} */(value);
const v8 = /**@type{string} */(value);

const v9 = /** @type {string} */
(value);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe for @type comments (for all JSDoc comments) better to keep it in one line

/cc @thorn0

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, why not do this for all comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks good. I'll fix.

// Input
const foo = /* foo */
  bar;

// Expected output😄
const foo = /* foo */ bar;

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

src/main/comments.js Outdated Show resolved Hide resolved
src/main/comments.js Outdated Show resolved Hide resolved
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks very good!

@thorn0
Copy link
Member

thorn0 commented Mar 6, 2020

getAssignmentLeftNode still looks too language-specific. Are you really sure this is something that can be relevant for other languages? Usually, when some specific situation with printing comments in JS needs to be addressed, only files in language-js are changed. I need to understand what makes this situation special, why it requires changes in main.

Besides, multiple comments inside assignments behave strangely:

Prettier pr-7709
Playground link

--parser babel

Input:

var a = /*1*/ /*2*/ /*3*/
    // 4
    x;

var a = /*1*/ /*2*/ /*3*/
    x;

var a = /*1*/ /*2*/ /*3*/
    /*4*/
    x;

Output:

var a /*1*/ /*2*/ =
  /*3*/
  // 4
  x;

var a /*1*/ /*2*/ = /*3*/ x;

var a /*1*/ /*2*/ =
  /*3*/
  /*4*/
  x;

contents,
hasNewline(options.originalText, options.locEnd(comment)) ? hardline : " "
]);
const leftNode =
Copy link
Member

@thorn0 thorn0 Mar 6, 2020

Choose a reason for hiding this comment

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

Can we probably just use comment.precedingNode here instead of leftNode? Checked that, turns out precedingNode is already deleted when this function is called.

Copy link
Member

@thorn0 thorn0 Mar 6, 2020

Choose a reason for hiding this comment

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

How about making this solution more general:

    const lineBreak = hasNewline(options.originalText, options.locEnd(comment))
      ? hasNewline(options.originalText, options.locStart(comment), {
          backwards: true
        })
        ? hardline
        : line
      : " ";

?

I see this breaks some tests, but let's discuss those cases. Maybe this behavior is okay for them too.

Copy link
Member

Choose a reason for hiding this comment

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

Input:

if (foo) /* baz */
  bar();

if (foo)
/* baz */
bar();

Current output:

if (foo)
  /* baz */
  bar();

if (foo)
  /* baz */
  bar();

Output with my proposed change:

if (foo) /* baz */ bar();

if (foo)
  /* baz */
  bar();

I would say it's okay to do this. If the user wants this comment to be on the next line, they can move it manually, and it will stay there.

Copy link
Member Author

Choose a reason for hiding this comment

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

@thorn0 Thank you! I did not know backwards option. Could you tell me what does that mean?

Copy link
Member

@thorn0 thorn0 Mar 6, 2020

Choose a reason for hiding this comment

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

It determines the direction, in which hasNewLine searches for a new line. By default, it searches the source code forward from the given index. With backwards: true it searches backwards.

hasNewline(options.originalText, options.locStart(comment), { backwards: true })

This returns true if there is a new line between the comment and the last non-whitespace character before the comment.

Copy link
Member

Choose a reason for hiding this comment

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

Would be good to hear from @evilebottnawi or @j-f1 if they're okay with this change.

@thorn0
Copy link
Member

thorn0 commented Mar 7, 2020

Looking at the snapshots, I found some changes that don't look good to me:

Input:

const kochabCooieGameOnOboleUnweave = // ???
      annularCooeedSplicesWalksWayWay;

const bifornCringerMoshedPerplexSawder = // !!!
  glimseGlyphsHazardNoopsTieTie +
  averredBathersBoxroomBuggyNurl -
  anodyneCondosMalateOverateRetinol;

Prettier 1.19.1
Playground link

Output:

const kochabCooieGameOnOboleUnweave = annularCooeedSplicesWalksWayWay; // ???

const bifornCringerMoshedPerplexSawder = // !!!
  glimseGlyphsHazardNoopsTieTie +
  averredBathersBoxroomBuggyNurl -
  anodyneCondosMalateOverateRetinol;

Prettier pr-7709
Playground link

Output:

const kochabCooieGameOnOboleUnweave =
  // ???
  annularCooeedSplicesWalksWayWay;

const bifornCringerMoshedPerplexSawder =
  // !!!
  glimseGlyphsHazardNoopsTieTie +
  averredBathersBoxroomBuggyNurl -
  anodyneCondosMalateOverateRetinol;

@thorn0
Copy link
Member

thorn0 commented Mar 7, 2020

I propose the following implementation of handleVariableDeclaratorComments to keep the existing formatting for those // comments (edited!):

function handleVariableDeclaratorComments(
  enclosingNode,
  followingNode,
  comment
) {
  if (
    enclosingNode &&
    (enclosingNode.type === "VariableDeclarator" ||
      enclosingNode.type === "AssignmentExpression") &&
    followingNode &&
    (followingNode.type === "ObjectExpression" ||
      followingNode.type === "ArrayExpression" ||
      followingNode.type === "TemplateLiteral" ||
      followingNode.type === "TaggedTemplateExpression" ||
      isBlockComment(comment))
  ) {
    addLeadingComment(followingNode, comment);
    return true;
  }
  return false;
}

Copy link
Member

@thorn0 thorn0 left a comment

Choose a reason for hiding this comment

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

Looks good now. Please add the snippet from #7709 (comment) to the tests.

@sosukesuzuki
Copy link
Member Author

@evilebottnawi @j-f1 Could you review again?

@thorn0 thorn0 merged commit 78fe9ca into prettier:next Mar 10, 2020
@sosukesuzuki sosukesuzuki deleted the comments-with-assignment branch March 10, 2020 11:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants