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

[handlebars] group params in opening block statements #14067

Merged

Conversation

jamescdavis
Copy link
Contributor

@jamescdavis jamescdavis commented Dec 27, 2022

Description

This is a follow-up to #13930 to establish wrapping consistency between opening block statements and else blocks by grouping params in opening blocks. This causes params to break to a new line together and not be split across lines unless the length of params exceeds the print width. This also updates the else block wrapping to behave exactly the same as opening blocks (after #13930 it attempts to keep params on same line as the "else block" keywords, even when block params cause wrapping, see else block in the Prettier main example below).

--print-width 80
{{! Input }}
{{#block param param param param param param param param param param as |blockParam|}}
  Hello
{{else block param param param param param param param param param param as |blockParam|}}
  There
{{/block}}

{{! Prettier main (includes unreleased #13930) }}
{{#block
  param
  param
  param
  param
  param
  param
  param
  param
  param
  param
  as |blockParam|
}}
  Hello
{{else block param param param param param param param param param param
  as |blockParam|
}}
  There
{{/block}}

{{! This PR }}
{{#block
  param param param param param param param param param param
  as |blockParam|
}}
  Hello
{{else block
  param param param param param param param param param param
  as |blockParam|
}}
  There
{{/block}}

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (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

--parser glimmer

@jamescdavis jamescdavis force-pushed the hbs_group_params_and_block_params_separately branch from 7e1d6c3 to 9112a12 Compare December 27, 2022 19:57
@jamescdavis jamescdavis changed the title [handlebars] group params and block params separately in block statements [handlebars] group params in block statements Dec 27, 2022
@jamescdavis jamescdavis force-pushed the hbs_group_params_and_block_params_separately branch from 9112a12 to f048ddc Compare December 27, 2022 20:11
@jamescdavis jamescdavis changed the title [handlebars] group params in block statements [handlebars] group params in opening block statements Dec 27, 2022
@jamescdavis jamescdavis marked this pull request as ready for review December 27, 2022 21:28
@jamescdavis jamescdavis force-pushed the hbs_group_params_and_block_params_separately branch from a91facb to e88b6c0 Compare December 27, 2022 21:33
@fisker fisker self-assigned this Dec 30, 2022
@fisker
Copy link
Sponsor Member

fisker commented Dec 30, 2022

@jamescdavis I don't use handlebars, but is this a bug?

Prettier 2.8.1
Playground link

--parser glimmer

Input:

{{#block a}}
  1
{{else if b}}
  2
{{else}}
  3
{{/block}}

Output:

{{#block a}}
  1
{{else}}{{#if b}}
    2
  {{else}}
    3
  {{/if}}{{/block}}

I didn't mean else if is splited, I mean shouldn't {{/block}} align with the opening?

@fisker
Copy link
Sponsor Member

fisker commented Dec 30, 2022

@jamescdavis I made some changes, please take a look.

@jamescdavis
Copy link
Contributor Author

@fisker, your changes look good. Thanks for simplifying the logic!

@jamescdavis
Copy link
Contributor Author

jamescdavis commented Dec 30, 2022

As far as the closing block alignment, it's due to the --html-whitespace-sensitivity setting in combination with prettier rewriting the else if.

When

{{else if b}}

is rewritten to

{{else}}{{#if b}}

and

{{/block}}

is rewritten to

  {{/if}}{{/block}}

and --html-whitespace-sensitivity is css or strict, it does not add whitespace between the statements.

See this playground link with --html-whitespace-sensitivity ignore.

You also see that explicitly writing this with newlines works fine, even with --html-whitespace-sensitivity strict: playground link

In any case, this would only be an issue when the {{else if is rewritten, which would only happen when the keyword that follows else doesn't match the opening block keyword. And TBH I'm not even sure that construct makes sense. When I did #13507, I made it only preserve the {{else foo when it matches the opening block (e.g. {{#foo) since it doesn't really make sense when they don't match.

If you wanted something like:

{{#block a}}
  1
{{else}}
  {{#if b}}
    2
  {{/if}}
{{/block}}

You'd explicitly write it that way.

And

{{#block a}}
  1
{{else}}
  {{#if b}}
    2
  {{/if}}
{{else}}
  3
{{/block}}

doesn't even make sense since there are two {{else}} blocks.

Maybe it should not try to match the opening block keyword and never split {{else anything regardless of the opening block? Prettier probably shouldn't change how the code will be interpreted by the compiler and so probably should just leave

{{#block a}}
  1
{{else if b}}
  2
{{else}}
  3
{{/block}}

as-is.

@jamescdavis
Copy link
Contributor Author

jamescdavis commented Dec 30, 2022

@fisker I did notice one issue after your changes: when the if (or other block keyword) in {{else if goes beyond the print width causing the if to wrap to the next line, the if is not indented properly.

--print-width 8

Input:

{{#if a}}
  1
{{else if b}}
  2
{{/if}}

Output:

{{#if
  a
}}
  1
{{else
if
  b
}}
  2
{{/if}}

Expected:

{{#if
  a
}}
  1
{{else
  if
  b
}}
  2
{{/if}}

I've added a failing test and a fix for this.

But also, maybe it shouldn't wrap here? Maybe else if should always stay on the same line, even when it exceeds the print width? Maybe I was overzealous with group? 😆 If I made that change then else if would be more consistent with if.

It makes more of a difference with longer block keywords. Which do you think is better? (assuming --print-width 80)

Input:

{{#abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrst param param param as |foo|}}
  1
{{else abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrst param param param as |bar|}}
  2
{{/abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrst}}

Ouput with else "if" grouped:

{{#abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrst
  param param param
  as |foo|
}}
  1
{{else
  abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrst
  param param param
  as |bar|
}}
  2
{{/abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrst}}

or

Output with else "if" not grouped:

{{#abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrst
  param param param
  as |foo|
}}
  1
{{else abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrst
  param param param
  as |bar|
}}
  2
{{/abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrstuvwxyz-abcdefghijklmnopqrst}}

Playground link

@jamescdavis
Copy link
Contributor Author

I actually decided to make the change to not group else if so that it never wraps the if (or other block keyword) to a new line. This feels better and is actually what happens now (param wrapping is mess on stable, but should be in good shape after this PR). LMK, if you don't agree.

@@ -570,6 +624,86 @@ printWidth: 80
Another thing
{{~/if~}}

<div>
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

You can put this test in a separate dir with small printWidth, instead of deep.nest.

@fisker
Copy link
Sponsor Member

fisker commented Dec 30, 2022

Prettier probably shouldn't change how the code will be interpreted by the compiler and so probably should just leave

Agree, but this problem can be addressed in another PR.

@fisker
Copy link
Sponsor Member

fisker commented Dec 30, 2022

I did notice one issue after your changes: when the if (or other block keyword) in {{else if goes beyond the print width causing the if to wrap to the next line, the if is not indented properly.

I removed indent() on it since indent should alway start with line, if you want indent(["else", line, "something else"]), you should write ["else", indent([line, "something else"])], but I can be wrong, to be honest, I don't really understand how indent works.

@fisker
Copy link
Sponsor Member

fisker commented Dec 30, 2022

Which do you think is better?

I nerver use handlebars, I can't tell.

@dcyriller Do you have time to review? We plan to make a release, and I was hoping this to be included.

Copy link
Collaborator

@dcyriller dcyriller left a comment

Choose a reason for hiding this comment

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

Sorry I have been afk for a few weeks.

@jamescdavis
Copy link
Contributor Author

@fisker I moved the test as you suggested. With the approval from @dcyriller, this should be GTG?

@fisker fisker merged commit 37fb53a into prettier:main Jan 28, 2023
@fisker
Copy link
Sponsor Member

fisker commented Jan 28, 2023

@jamescdavis Thanks for your work.
@dcyriller Thanks for your time too.

@jamescdavis jamescdavis deleted the hbs_group_params_and_block_params_separately branch January 28, 2023 03:07
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Feb 10, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [prettier](https://prettier.io) ([source](https://github.com/prettier/prettier)) | devDependencies | patch | [`2.8.3` -> `2.8.4`](https://renovatebot.com/diffs/npm/prettier/2.8.3/2.8.4) |

---

### Release Notes

<details>
<summary>prettier/prettier</summary>

### [`v2.8.4`](https://github.com/prettier/prettier/blob/HEAD/CHANGELOG.md#&#8203;284)

[Compare Source](prettier/prettier@2.8.3...2.8.4)

[diff](prettier/prettier@2.8.3...2.8.4)

##### Fix leading comments in mapped types with `readonly` ([#&#8203;13427](prettier/prettier#13427) by [@&#8203;thorn0](https://github.com/thorn0), [@&#8203;sosukesuzuki](https://github.com/sosukesuzuki))

<!-- prettier-ignore -->

```tsx
// Input
type Type = {
  // comment
  readonly [key in Foo];
};

// Prettier 2.8.3
type Type = {
  readonly // comment
  [key in Foo];
};

// Prettier 2.8.4
type Type = {
  // comment
  readonly [key in Foo];
};
```

##### Group params in opening block statements ([#&#8203;14067](prettier/prettier#14067) by [@&#8203;jamescdavis](https://github.com/jamescdavis))

This is a follow-up to [#&#8203;13930](prettier/prettier#13930) to establish wrapping consistency between opening block statements and else blocks by
grouping params in opening blocks. This causes params to break to a new line together and not be split across lines
unless the length of params exceeds the print width. This also updates the else block wrapping to behave exactly the
same as opening blocks.

<!-- prettier-ignore -->

```hbs
{{! Input }}
{{#block param param param param param param param param param param as |blockParam|}}
  Hello
{{else block param param param param param param param param param param as |blockParam|}}
  There
{{/block}}

{{! Prettier 2.8.3 }}
{{#block
  param
  param
  param
  param
  param
  param
  param
  param
  param
  param
  as |blockParam|
}}
  Hello
{{else block param
param
param
param
param
param
param
param
param
param}}
  There
{{/block}}

{{! Prettier 2.8.4 }}
{{#block
  param param param param param param param param param param
  as |blockParam|
}}
  Hello
{{else block
  param param param param param param param param param param
  as |blockParam|
}}
  There
{{/block}}
```

##### Ignore files in `.sl/` ([#&#8203;14206](prettier/prettier#14206) by [@&#8203;bolinfest](https://github.com/bolinfest))

In [Sapling SCM](https://sapling-scm.com/), `.sl/` is the folder where it stores its state, analogous to `.git/` in Git. It should be ignored in Prettier like the other SCM folders.

##### Recognize `@satisfies` in Closure-style type casts ([#&#8203;14262](prettier/prettier#14262) by [@&#8203;fisker](https://github.com/fisker))

<!-- prettier-ignore -->

```jsx
// Input
const a = /** @&#8203;satisfies {Record<string, string>} */ ({hello: 1337});
const b = /** @&#8203;type {Record<string, string>} */ ({hello: 1337});

// Prettier 2.8.3
const a = /** @&#8203;satisfies {Record<string, string>} */ { hello: 1337 };
const b = /** @&#8203;type {Record<string, string>} */ ({ hello: 1337 });

// Prettier 2.8.4
const a = /** @&#8203;satisfies {Record<string, string>} */ ({hello: 1337});
const b = /** @&#8203;type {Record<string, string>} */ ({hello: 1337});
```

##### Fix parens in inferred function return types with `extends` ([#&#8203;14279](prettier/prettier#14279) by [@&#8203;fisker](https://github.com/fisker))

<!-- prettier-ignore -->

```ts
// Input
type Foo<T> = T extends ((a) => a is infer R extends string) ? R : never;

// Prettier 2.8.3 (First format)
type Foo<T> = T extends (a) => a is infer R extends string ? R : never;

// Prettier 2.8.3 (Second format)
SyntaxError: '?' expected.

// Prettier 2.8.4
type Foo<T> = T extends ((a) => a is infer R extends string) ? R : never;
```

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMjUuMSIsInVwZGF0ZWRJblZlciI6IjM0LjEyOC4zIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1775
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Jan 4, 2024
Co-authored-by: fisker Cheung <lionkay@gmail.com>
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.

None yet

3 participants