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(js_formatter): fix some random indention inconsistencies #1162

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

faultyserver
Copy link
Contributor

Summary

Just fixing two little issues I found running Biome on a large codebase. Commenting inline on each fix below.

Test Plan

Managed to remove another diff snapshot :)

Copy link

netlify bot commented Dec 12, 2023

Deploy Preview for biomejs canceled.

Name Link
🔨 Latest commit 0d6fed9
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/6578c7852f8c2900086daac2

@github-actions github-actions bot added A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Dec 12, 2023
&& f.context().comments().has_leading_comments(arrow.syntax());
let is_first = is_first_in_chain;

let formatted_signature = format_with(|f: &mut JsFormatter| {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole diff is a little bit weird, but this just moves the leading comment inside of the signature formatting, ensuring that it picks up the potential indention that's applied below (line +590).

It fixes this:

foo(
  () =>
  // a comment
    () => {}
)

which should actually be formatted to

foo(
  () =>
    // a comment
    () => {}
)

where the leading comment lines up with the following parens.

@@ -530,6 +530,7 @@ impl AnyJsBinaryLikeExpression {
| JsSyntaxKind::JS_THROW_STATEMENT
| JsSyntaxKind::JS_CALL_EXPRESSION
| JsSyntaxKind::JS_IMPORT_CALL_EXPRESSION
| JsSyntaxKind::JS_CALL_ARGUMENT_LIST
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a binary expression is part of the call arguments, its grand parent is JsCallArgumentList, not just JsCallExpression. These intermediary nodes make it kind of hard to compare directly to prettier, but come up consistently (other places also have to check like JsParameterList instead of JsFunctionDeclaration, for example).

Adding this here means that binaries are formatted consistently whether they're the receiver or an argument to a call expression:

foo(
  someCondition === true ||
  anotherCondition === false
);

should actually indent the right hand side, but wasn't because the grandparent is JsCallArgumentList, but this was only checking for JsCallExpression directly. Now it matches Prettier as:

foo(
  someCondition === true ||
    anotherCondition === false
);

@Conaclos Conaclos merged commit 477e9d9 into main Dec 12, 2023
18 checks passed
@Conaclos Conaclos deleted the faulty/random-indents branch December 12, 2023 21:04
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants