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(formatter): incompatibility for class and implements #4183

Closed
wants to merge 3 commits into from

Conversation

mdm317
Copy link
Contributor

@mdm317 mdm317 commented Oct 6, 2024

fixes #4143

Summary

  "class",
  group("#1", [
    indent([
      " LongClassName",
      soft_line_break_or_space,
      "implements",
      group([
        indent([
          soft_line_break_or_space,
          "InterfaceNameLengthIsSixtySixCharacterssssssssssssssssssssssssssss"
        ])
      ])
    ])
  ]),
  " ",  <===== this
  if_group_breaks("#1", [hard_line_break]),
  "{",
  indent([hard_line_break, " constructer", group(["()"]), " {}"]),
  hard_line_break,
  "}",

When checking the operation of Prettier, the fits function calculates the width of an empty string as 1 instead of 0. The empty string refers to line 15 in the code, and we are using it as a space.
So i changed space to hardspace

Test Plan

add test case

@github-actions github-actions bot added A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Oct 6, 2024
@ah-yu
Copy link
Contributor

ah-yu commented Oct 6, 2024

Why Prettier breaks implements ... even if the line doesn't exceed the width limit.

Prettier's playground has a ruler, making it easier to check the line width.

image

@mdm317
Copy link
Contributor Author

mdm317 commented Oct 6, 2024

Thank you for asking.

I also thought it strange at first, so I checked how Prettier was working.
But, I'm not very familiar with the Prettier library, and I don't know whether this is what they intended or if it's a bug.

Prettier would sometimes break a line even when it hadn't exceeded the line length.
It seems like the example I worked on before was one of those cases. previous pr

Do you think this issue is a Prettier error rather than something we need to change?

Copy link

codspeed-hq bot commented Oct 6, 2024

CodSpeed Performance Report

Merging #4183 will degrade performances by 8.71%

Comparing mdm317:fix/format-class-and-implements (035bd40) with main (35bb699)

Summary

❌ 1 regressions
✅ 104 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main mdm317:fix/format-class-and-implements Change
db_17847247775464589309.json[cached] 12.8 ms 14 ms -8.71%

@ah-yu
Copy link
Contributor

ah-yu commented Oct 6, 2024

Do you think this issue is a Prettier error rather than something we need to change?

I personally tend to think this is a bug in Prettier.

@mdm317
Copy link
Contributor Author

mdm317 commented Oct 6, 2024

Thank you for sharing your thoughts! @ah-yu

@dyc3
Could you also share your thoughts?

@dyc3
Copy link
Contributor

dyc3 commented Oct 6, 2024

I also feel like this is a bug in prettier, but I also think we should generally try to match prettier. The fix here doesn't seem that complicated.

@Conaclos
Copy link
Member

Conaclos commented Oct 6, 2024

We could open an issue on the Prettier repository.

@dyc3
Copy link
Contributor

dyc3 commented Oct 6, 2024

I've opened prettier/prettier#16723

@@ -8,6 +8,14 @@ info: ts/class/implements_clause.ts
class ClassName implements Interface { }

class LongClassName implements Interface1, Interface2, Interface3, Interface4, Interface5 { }

class LongClassName implements InterfaceNameLengthIsSixtySixCharacterssssssssssssssssssssssssssss {
constructer(){}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: typo

@ematipico
Copy link
Member

Closing, it's a prettier bug and they are fixing it

@ematipico ematipico closed this Oct 16, 2024
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.

📝 Formatting incompatibility between biome and prettier for class and implements
6 participants