Skip to content

Fix comments handling for for-statements that body is expression statements#9829

Closed
sosukesuzuki wants to merge 5 commits intoprettier:mainfrom
sosukesuzuki:fix-9812
Closed

Fix comments handling for for-statements that body is expression statements#9829
sosukesuzuki wants to merge 5 commits intoprettier:mainfrom
sosukesuzuki:fix-9812

Conversation

@sosukesuzuki
Copy link
Copy Markdown
Contributor

@sosukesuzuki sosukesuzuki commented Dec 6, 2020

Description

Fixes #9812

Checklist

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

@fisker
Copy link
Copy Markdown
Member

fisker commented Dec 23, 2020

Excluding ExpressionStatement seems a good way, but should we do something about BlockStatement and EmptyStatement?

Prettier pr-9829
Playground link

--parser babel

Input:

for (const p of ['fullName', 'organ', 'position', 'rank'])
// @ts-expect-error
{form.setValue(`${prefix}.data.${p}`, response[p])}

for (const p of ['fullName', 'organ', 'position', 'rank'])
// @ts-expect-error
{}

for (const p of ['fullName', 'organ', 'position', 'rank'])
// @ts-expect-error
;

Output:

// @ts-expect-error
for (const p of ["fullName", "organ", "position", "rank"]) {
  form.setValue(`${prefix}.data.${p}`, response[p]);
}

// @ts-expect-error
for (const p of ["fullName", "organ", "position", "rank"]) {
}

for (const p of ["fullName", "organ", "position", "rank"]);
// @ts-expect-error

@sosukesuzuki
Copy link
Copy Markdown
Contributor Author

@fisker Yes, but it's inconsistent with if, for and switch. I'll create another PR to make them consistent after this is merged.

Prettier 2.2.1
Playground link

--parser babel

Input:

if (foo) {
  // foo
}

// foo
for (;;) {}

switch (
  bar
  // foo
) {
}

Output:

if (foo) {
  // foo
}

// foo
for (;;) {}

switch (
  bar
  // foo
) {
}

@thorn0
Copy link
Copy Markdown
Member

thorn0 commented Jan 12, 2021

Did you try not to special-case ExpressionStatement and instead check that the following node isn't the body of the loop?

This handleForComments function was added in #901 to solve #886.

Extending its effect to ForStatement nodes doesn't seem to be a good idea:

Prettier pr-9829
Playground link

--parser babel

Input:

for (
  a = 1;
  // this condition is tricky:
  a === b || a === c;
  a++
) {
  console.log()
}

Output:

// this condition is tricky:
for (a = 1; a === b || a === c; a++) {
  console.log();
}

Would be good to find a way to remove this function and find a better solution for #886 as the current one can change the order of comments:

Prettier 2.2.1
Playground link

--parser babel

Input:

for (/*a*/a
/*b*/
of b) break

Output:

/*b*/
for (/*a*/ a of b) break;

@thorn0
Copy link
Copy Markdown
Member

thorn0 commented Jan 12, 2021

These issues related to comments are really hard. Fixing individual edge cases we seem to be never sure we don't break other edge cases at the same time.

Base automatically changed from master to main January 23, 2021 17:13
@fisker fisker mentioned this pull request Oct 18, 2025
4 tasks
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.

@ts-expect-error comment is being shifted so it breaks

3 participants