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

feat(biome_js_formatter): introduce the new objectWrap option #5068

Open
wants to merge 11 commits into
base: next
Choose a base branch
from

Conversation

siketyan
Copy link
Contributor

@siketyan siketyan commented Feb 9, 2025

Summary

Prettier 3.5 introduced a new Object Wrap option. This improves code consistency, so it will be great if Biome also support this option.

This pull request introduces the objectWrap option to enforce collapsing object literals when possible.

Test Plan

Snapshot test cases added.

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Feb 9, 2025
@siketyan siketyan changed the title Feat/object wrap feat(biome_js_formatter): introduce thew new objectWrap option Feb 9, 2025
@siketyan siketyan changed the title feat(biome_js_formatter): introduce thew new objectWrap option feat(biome_js_formatter): introduce the new objectWrap option Feb 9, 2025
Copy link

codspeed-hq bot commented Feb 9, 2025

CodSpeed Performance Report

Merging #5068 will not alter performance

Comparing siketyan:feat/object-wrap (b44fc53) with next (8379a60)

Summary

✅ 94 untouched benchmarks

Copy link
Contributor

@dyc3 dyc3 left a comment

Choose a reason for hiding this comment

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

Looking good, just a couple minor things.

Comment on lines 582 to 583
"as-needed" => Ok(Self::Preserve),
"always" => Ok(Self::Collapse),
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right.

Suggested change
"as-needed" => Ok(Self::Preserve),
"always" => Ok(Self::Collapse),
"preserve" => Ok(Self::Preserve),
"collapse" => Ok(Self::Collapse),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, thank you for catching this. Fixed in 5eb499b

@@ -103,6 +103,11 @@ pub struct JsFormatterConfiguration {
#[bpaf(long("bracket-spacing"), argument("true|false"))]
#[serde(skip_serializing_if = "Option::is_none")]
pub bracket_spacing: Option<BracketSpacing>,

/// Whether to enforce collapsing object literals when possible. Defaults to preserve.
#[bpaf(long("object-wrap"), argument("preserve|collapse"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

This argument should be prefixed with javascript, like the args are since this is javascript specific. eg --javascript-object-wrap

Suggested change
#[bpaf(long("object-wrap"), argument("preserve|collapse"))]
#[bpaf(long("javascript-object-wrap"), argument("preserve|collapse"))]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f220b66.

@siketyan siketyan requested a review from dyc3 February 10, 2025 01:37
@ematipico
Copy link
Member

I don't see any new snapshots added. Can you please point me to those?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Formatter Area: formatter A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants