Skip to content

fix(formatter): dedent logic inconsistent with prettier where the indent-style is space and the indent-width is not 2.#2134

Merged
ah-yu merged 19 commits into
biomejs:mainfrom
mdm317:fix/dedent-when-indentstyle-is-space
Apr 10, 2024
Merged

fix(formatter): dedent logic inconsistent with prettier where the indent-style is space and the indent-width is not 2.#2134
ah-yu merged 19 commits into
biomejs:mainfrom
mdm317:fix/dedent-when-indentstyle-is-space

Conversation

@mdm317

@mdm317 mdm317 commented Mar 19, 2024

Copy link
Copy Markdown
Contributor

Summary

I created a stack named 'indent_stack' to store the records of indentations in order to remove the nearest indention when StartDedent and restore indention when EndDedent

Fixes #2037

Test Plan

I couldn't add test code because errors occurred only in settings where the indent-style is space and the indent-width is not 2.
We can check through the playground.

@github-actions github-actions Bot added A-Formatter Area: formatter A-Website Area: website A-Changelog Area: changelog labels Mar 19, 2024
@netlify

netlify Bot commented Mar 19, 2024

Copy link
Copy Markdown

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 23010d6
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/661568e3fdbbf20008d3323e
😎 Deploy Preview https://deploy-preview-2134--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 99 (🔴 down 1 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@codspeed-hq

codspeed-hq Bot commented Mar 19, 2024

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #2134 will not alter performance

Comparing mdm317:fix/dedent-when-indentstyle-is-space (23010d6) with main (6ec8710)

Summary

✅ 93 untouched benchmarks

@ematipico ematipico left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you, @mdm317, for tackling the issue. I left some suggestions. One important thing you should add is tests. Can you create some?

Comment thread crates/biome_formatter/src/printer/call_stack.rs Outdated
Comment thread crates/biome_formatter/src/printer/call_stack.rs Outdated
Comment thread crates/biome_formatter/src/printer/call_stack.rs Outdated
Comment thread crates/biome_formatter/src/printer/call_stack.rs Outdated
@mdm317

mdm317 commented Mar 20, 2024

Copy link
Copy Markdown
Contributor Author

Thank you for the review!!
I will investigate changing prettier options for testing or using a different approach for testing.
Please give me more time

@Sec-ant

Sec-ant commented Mar 24, 2024

Copy link
Copy Markdown
Contributor

@mdm317 I think we can just put the test cases in biome_cli tests for now: biome/crates/biome_cli/tests/commands/format.rs. There're existing test cases there to show to how add a config file to the memory fs.

And #2168 seems also a useful test case for this fix. You may want to check it out.

@github-actions github-actions Bot added the A-CLI Area: CLI label Mar 25, 2024
@mdm317

mdm317 commented Mar 25, 2024

Copy link
Copy Markdown
Contributor Author

Big thanks @Sec-ant. With your help, I was able to add test to the biome_cli

Comment thread crates/biome_formatter/src/printer/mod.rs
Comment thread crates/biome_formatter/src/printer/mod.rs

@ah-yu ah-yu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! Left some nitpicks

Comment thread crates/biome_cli/tests/commands/format.rs Outdated
Comment thread crates/biome_formatter/src/builders.rs
Comment thread crates/biome_formatter/src/printer/mod.rs Outdated
Comment thread crates/biome_formatter/src/printer/mod.rs Outdated
Comment thread crates/biome_formatter/src/printer/mod.rs Outdated
Comment thread CHANGELOG.md Outdated
@github-actions github-actions Bot added the L-JavaScript Language: JavaScript and super languages label Apr 8, 2024

@ematipico ematipico left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great contribution @mdm317, and amazing reviews @ah-yu and @Sec-ant! ❤️

@ah-yu ah-yu merged commit 17641a1 into biomejs:main Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Changelog Area: changelog A-CLI Area: CLI A-Formatter Area: formatter A-Website Area: website L-JavaScript Language: JavaScript and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

📝 Inconsistent print behavior of nested aligns with Prettier

4 participants