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 comments handling for for-statements that body is expression statements #9829

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

Conversation

sosukesuzuki
Copy link
Member

@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
Sponsor 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
Member 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
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
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
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