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: Fix indentation for binary expressions with trailing comments #8476

Merged
merged 19 commits into from Jun 4, 2020

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Jun 2, 2020

Fixes #8451
This PR seems to work fine but the doc is transformed so it maybe a dirty way...

  • 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
Copy link
Member

fisker commented Jun 2, 2020

Seems dirty, can it done in printBinaryishExpressions?

@sosukesuzuki
Copy link
Member Author

okay, I'll try it later.

@sosukesuzuki
Copy link
Member Author

done

@fisker
Copy link
Member

fisker commented Jun 2, 2020

I mean can we print the first part differently in that function?

@thorn0
Copy link
Member

thorn0 commented Jun 2, 2020

Performance-wise, it was better before the change. printBinaryishExpressions is recursive, and we don't need to do this flattening on each recursion level.

@thorn0
Copy link
Member

thorn0 commented Jun 2, 2020

Prettier pr-8476
Playground link

--parser babel

Input:

bifornCringerMoshedPerplexSawder(
  askTrovenaBeenaDependsRowans,
  glimseGlyphsHazardNoopsTieTie,
  averredBathersBoxroomBuggyNurl
) 
|> kochabCooieGameOnOboleUnweave
|> glimseGlyphsHazardNoopsTieTie;

bifornCringerMoshedPerplexSawder(
  askTrovenaBeenaDependsRowans,
  glimseGlyphsHazardNoopsTieTie,
  averredBathersBoxroomBuggyNurl
) // comment
|> kochabCooieGameOnOboleUnweave
|> glimseGlyphsHazardNoopsTieTie;

Output:

bifornCringerMoshedPerplexSawder(
  askTrovenaBeenaDependsRowans,
  glimseGlyphsHazardNoopsTieTie,
  averredBathersBoxroomBuggyNurl
)
  |> kochabCooieGameOnOboleUnweave
  |> glimseGlyphsHazardNoopsTieTie;

bifornCringerMoshedPerplexSawder(
    askTrovenaBeenaDependsRowans,
    glimseGlyphsHazardNoopsTieTie,
    averredBathersBoxroomBuggyNurl
  )  // comment
  |> kochabCooieGameOnOboleUnweave
  |> glimseGlyphsHazardNoopsTieTie;

@sosukesuzuki
Copy link
Member Author

@fisker

I mean can we print the first part differently in that function?

I tried but I was not sure how to print it differently...

@thorn0

Performance-wise, it was better before the change. printBinaryishExpressions is recursive, and we don't need to do this flattening on each recursion level.

the flattening is actually done only when the parent node type is ExpressionStatement so I think this is no problem for performance.

@thorn0
Copy link
Member

thorn0 commented Jun 2, 2020

@sosukesuzuki I'd try to do flattening where printComments is called. parts is an array everywhere, assigning it the result of the printComments call, which is a Doc { type: "concat" }, doesn't look right. See also #8451 (comment)

if (isBinaryish(node) && !node.comments) {
return hasTrailingCommentInBinaryish(node.left);
}
return isBinaryish(node) && hasTrailingComment(node);
Copy link
Member

Choose a reason for hiding this comment

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

Prettier pr-8476
Playground link

--parser babel

Input:

bifornCringerMoshedPerplexSawder(
  askTrovenaBeenaDependsRowans,
  glimseGlyphsHazardNoopsTieTie
)
|> foo
|> kochabCooieGameOnOboleUnweave
|> glimseGlyphsHazardNoopsTieTie;

bifornCringerMoshedPerplexSawder(
  askTrovenaBeenaDependsRowans,
  glimseGlyphsHazardNoopsTieTie
)
|> foo // comment
|> kochabCooieGameOnOboleUnweave
|> glimseGlyphsHazardNoopsTieTie;

Output:

bifornCringerMoshedPerplexSawder(
  askTrovenaBeenaDependsRowans,
  glimseGlyphsHazardNoopsTieTie
)
  |> foo
  |> kochabCooieGameOnOboleUnweave
  |> glimseGlyphsHazardNoopsTieTie;

bifornCringerMoshedPerplexSawder(
    askTrovenaBeenaDependsRowans,
    glimseGlyphsHazardNoopsTieTie
  )
  |> foo  // comment
  |> kochabCooieGameOnOboleUnweave
  |> glimseGlyphsHazardNoopsTieTie;

@sosukesuzuki
Copy link
Member Author

@thorn0 How do you think about adding an option named shouldReturnParts to comments.printComments. See 4ce5bf3

@thorn0
Copy link
Member

thorn0 commented Jun 3, 2020

Looks complicated and I'm not sure it's okay to circumvent the prependCursorPlaceholder calls (need to investigate, I never used formatWithCursor).

I have another idea. There is a function called normalizeParts in language-html/utils. It isn't specific to HTML, so let's extract it to document/doc-utils and call it on the result of our printComments call.

@fisker

This comment has been minimized.

@sosukesuzuki sosukesuzuki marked this pull request as draft June 3, 2020 13:29
// We group here when the callee is itself a call expression.
// See `isLongCurriedCallExpression` for more info.
isCallOrOptionalCallExpression(n.callee) ||
isBinaryish(path.getParentNode())
Copy link
Member

Choose a reason for hiding this comment

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

No, we can't tackle it this way:

Prettier pr-8476
Playground link

--parser babel

Input:

bifornCringerMoshedPerplexSawder[
  askTrovenaBeenaDependsRowans +
  glimseGlyphsHazardNoopsTieTie
]
  |> foo  // comment
  |> kochabCooieGameOnOboleUnweave
  |> glimseGlyphsHazardNoopsTieTie;

Output:

bifornCringerMoshedPerplexSawder[
    askTrovenaBeenaDependsRowans + glimseGlyphsHazardNoopsTieTie
  ]
  |> foo  // comment
  |> kochabCooieGameOnOboleUnweave
  |> glimseGlyphsHazardNoopsTieTie;

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, I'm fixing it.:+1:

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 d4e5e6c

@sosukesuzuki sosukesuzuki marked this pull request as ready for review June 3, 2020 14:41
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. Fixes this place in needs-parens.js:

if (
(parent.type === "ArrowFunctionExpression" &&
parent.body === node &&
node.type !== "SequenceExpression" && // these have parens added anyway
util.startsWithNoLookaheadToken(
node,
/* forbidFunctionClassAndDoExpr */ false
)) ||
(parent.type === "ExpressionStatement" &&
util.startsWithNoLookaheadToken(
node,
/* forbidFunctionClassAndDoExpr */ true
))
) {

}

return newParts;
}
Copy link
Member

Choose a reason for hiding this comment

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

We export this file to plugins, so I think we should documented it

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I was wrong, we don't have documentation about utils

Copy link
Member

Choose a reason for hiding this comment

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

This is definitely out of scope for this PR. The entire prettier.doc namespace isn't documented.

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.

👍

@thorn0
Copy link
Member

thorn0 commented Jun 3, 2020

@sosukesuzuki I think we came close to fixing #1210.

@sosukesuzuki sosukesuzuki changed the title JavaScript: Align indentation for binary/logical expression statements with trailing comments JavaScript: Fix indentation for binary expressions with trailing comments Jun 3, 2020
@sosukesuzuki

This comment has been minimized.

@fisker
Copy link
Member

fisker commented Jun 4, 2020

With block comment seems ugly

Prettier pr-8476
Playground link

--parser babel

Input:

bifornCringerMoshedPerplexSawder
|> foo 
|> foo /* comment */
|> foo /* comment */
|> kochabCooieGameOnOboleUnweave
|> glimseGlyphsHazardNoopsTieTie;

Output:

bifornCringerMoshedPerplexSawder
  |> foo
  |> foo |>
  /* comment */
  foo |>
  /* comment */
  kochabCooieGameOnOboleUnweave
  |> glimseGlyphsHazardNoopsTieTie;

Prettier 2.0.5
Playground link

--parser babel

Input:

bifornCringerMoshedPerplexSawder
|> foo 
|> foo /* comment */
|> foo /* comment */
|> kochabCooieGameOnOboleUnweave
|> glimseGlyphsHazardNoopsTieTie;

Output:

bifornCringerMoshedPerplexSawder
|> foo
|> foo /* comment */
|> foo /* comment */
  |> kochabCooieGameOnOboleUnweave
  |> glimseGlyphsHazardNoopsTieTie;

@thorn0
Copy link
Member

thorn0 commented Jun 4, 2020

@fisker This is an oversight from #7787. I'm going to revert it.

@thorn0 thorn0 merged commit 9c67e7b into prettier:master Jun 4, 2020
@sosukesuzuki sosukesuzuki deleted the 8451 branch June 4, 2020 06:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 5, 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.

Comment breaks pipeline and logic operator indention
4 participants