Skip to content

Commit

Permalink
feat(rome_js_formatter): Break long object literal members over two l…
Browse files Browse the repository at this point in the history
…ines rome#2425
  • Loading branch information
denbezrukov committed May 30, 2022
1 parent 9be13ca commit 2d8663c
Show file tree
Hide file tree
Showing 17 changed files with 351 additions and 170 deletions.
181 changes: 177 additions & 4 deletions crates/rome_js_formatter/src/js/objects/property_object_member.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
use crate::prelude::*;

use crate::FormatNodeFields;
use rome_js_syntax::JsAnyExpression;
use rome_js_syntax::JsAnyLiteralExpression;
use rome_js_syntax::JsCallExpressionFields;
use rome_js_syntax::JsConditionalExpressionFields;
use rome_js_syntax::JsPropertyObjectMember;
use rome_js_syntax::JsPropertyObjectMemberFields;
use rome_js_syntax::JsSyntaxKind;
use rome_js_syntax::JsSyntaxNode;
use rome_rowan::SyntaxResult;
use rome_rowan::TextSize;

impl FormatNodeFields<JsPropertyObjectMember> for FormatNodeRule<JsPropertyObjectMember> {
fn format_fields(
Expand All @@ -14,10 +22,175 @@ impl FormatNodeFields<JsPropertyObjectMember> for FormatNodeRule<JsPropertyObjec
colon_token,
value,
} = node.as_fields();
let layout = get_object_member_layout(formatter, node)?;

let key = name.format();
let colon = colon_token.format();
let value = value.format();
formatted![formatter, [key, colon, space_token(), value]]
let formatted = match layout {
PropertyObjectMemberLayoutMode::Fluid => {
let group_id = formatter.group_id("property_object_member");

let value = formatted![formatter, [value.format()]]?;
formatted![
formatter,
[
group_elements(formatted![formatter, [name.format()]]?),
colon_token.format(),
group_elements_with_options(
indent(soft_line_break_or_space()),
GroupElementsOptions {
group_id: Some(group_id)
}
),
line_suffix_boundary(),
if_group_with_id_breaks(indent(value.clone()), group_id),
if_group_with_id_fits_on_line(value, group_id),
]
]
}
PropertyObjectMemberLayoutMode::BreakAfterOperator => formatted![
formatter,
[
group_elements(formatted![formatter, [name.format()]]?),
colon_token.format(),
space_token(),
group_elements(formatted![
formatter,
[indent(soft_line_break_or_space()), value.format()]
]?),
]
],
PropertyObjectMemberLayoutMode::NeverBreakAfterOperator => formatted![
formatter,
[
group_elements(formatted![formatter, [name.format()]]?),
colon_token.format(),
space_token(),
value.format()
]
],
};

Ok(group_elements(formatted?))
}
}

enum PropertyObjectMemberLayoutMode {
Fluid,
BreakAfterOperator,
NeverBreakAfterOperator,
}

fn get_object_member_layout(
formatter: &Formatter<JsFormatOptions>,
node: &JsPropertyObjectMember,
) -> SyntaxResult<PropertyObjectMemberLayoutMode> {
let JsPropertyObjectMemberFields {
name,
colon_token: _,
value,
} = node.as_fields();

let name = name?;
let value = value?;

let value = match value {
JsAnyExpression::TsAsExpression(expression) => expression.expression()?,
_ => value,
};

const MIN_OVERLAP_FOR_BREAK: u8 = 3;

let text_width_for_break = (formatter.options().tab_width() + MIN_OVERLAP_FOR_BREAK) as u32;
let has_short_key = name.range().len() < TextSize::from(text_width_for_break);

if is_break_after_operator(&value, has_short_key)? {
return Ok(PropertyObjectMemberLayoutMode::BreakAfterOperator);
}

if is_never_break_after_operator(&value, has_short_key)? {
return Ok(PropertyObjectMemberLayoutMode::NeverBreakAfterOperator);
}

Ok(PropertyObjectMemberLayoutMode::Fluid)
}

fn is_break_after_operator(value: &JsAnyExpression, has_short_key: bool) -> SyntaxResult<bool> {
if is_binaryish_expression(value.syntax()) {
return Ok(true);
}

if matches!(value, JsAnyExpression::JsSequenceExpression(_)) {
return Ok(true);
}

if let JsAnyExpression::JsConditionalExpression(conditional) = &value {
let JsConditionalExpressionFields {
test,
question_mark_token: _,
consequent: _,
colon_token: _,
alternate: _,
} = conditional.as_fields();

if is_binaryish_expression(test?.syntax()) {
return Ok(true);
}
}

if has_short_key {
return Ok(false);
}

if let JsAnyExpression::JsAnyLiteralExpression(
JsAnyLiteralExpression::JsStringLiteralExpression(_),
) = &value
{
return Ok(true);
}

Ok(false)
}

fn is_never_break_after_operator(
value: &JsAnyExpression,
has_short_key: bool,
) -> SyntaxResult<bool> {
if let JsAnyExpression::JsCallExpression(call_expression) = &value {
let JsCallExpressionFields {
callee,
optional_chain_token: _,
type_arguments: _,
arguments: _,
} = call_expression.as_fields();

if callee?.syntax().kind() == JsSyntaxKind::REQUIRE_KW {
return Ok(true);
}
}

if has_short_key {
return Ok(true);
}

if matches!(
value,
JsAnyExpression::JsClassExpression(_)
| JsAnyExpression::JsTemplate(_)
| JsAnyExpression::JsAnyLiteralExpression(
JsAnyLiteralExpression::JsBooleanLiteralExpression(_),
)
| JsAnyExpression::JsAnyLiteralExpression(
JsAnyLiteralExpression::JsNumberLiteralExpression(_)
)
) {
return Ok(true);
}

Ok(false)
}

fn is_binaryish_expression(node: &JsSyntaxNode) -> bool {
matches!(
node.kind(),
JsSyntaxKind::JS_BINARY_EXPRESSION | JsSyntaxKind::JS_LOGICAL_EXPRESSION
)
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/rome_js_formatter/tests/spec_test.rs
assertion_line: 244
expression: sequence_expression.js
---
# Input
Expand Down Expand Up @@ -74,15 +73,16 @@ function a() {
}

const object = {
something: (
____________first,
____________second,
____________third,
____________third,
____________third,
____________third,
____________third
),
something:
(
____________first,
____________second,
____________third,
____________third,
____________third,
____________third,
____________third
),
};

aLongIdentifierName,
Expand Down
12 changes: 6 additions & 6 deletions crates/rome_js_formatter/tests/specs/jsx/attributes.jsx.snap
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
---
source: crates/rome_js_formatter/tests/spec_test.rs
assertion_line: 242
expression: attributes.jsx

---
# Input

Expand Down Expand Up @@ -67,7 +65,8 @@ Quote style: Double Quotes
fontSize: 12,
height: "40vh",
overflowY: "scroll",
fontFamily: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
fontFamily:
"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
}}
/>;

Expand All @@ -89,7 +88,8 @@ Quote style: Double Quotes
fontSize: 12,
height: "50vh",
overflowY: "scroll",
fontFamily: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
fontFamily:
"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
}}
/>;

Expand All @@ -115,6 +115,6 @@ let a = <test

## Lines exceeding width of 80 characters

9: fontFamily: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
31: fontFamily: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
10: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
33: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",

Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,11 @@ const t = {
```js
const t = {
"hello": world(),
"this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line": orMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig(),
"this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line":
orMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig(),
"can-someone-explain": this(),
};
```

# Lines exceeding max width of 80 characters
```
3: "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line": orMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig(),
```

Original file line number Diff line number Diff line change
Expand Up @@ -193,13 +193,15 @@ const create2 = () => {
};
const obj = {
state: shouldHaveState &&
state:
shouldHaveState &&
stateIsOK &&
{
loadState: LOADED,
opened: false,
},
loadNext: (stateIsOK && hasNext) || {
loadNext:
(stateIsOK && hasNext) || {
skipNext: true,
},
loaded: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,8 @@ followersForUser(
// Followers: ["STEVE","SUZY"]
const mapStateToProps = (state) => ({
users: R.compose(
R.filter(R.propEq("status", "active")),
R.values,
)(state.users),
users:
R.compose(R.filter(R.propEq("status", "active")), R.values)(state.users),
});
```
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,23 @@ true
# Output
```js
({
processors: [
require(
"autoprefixer",
{
browsers: ["> 1%", "last 2 versions", "ie >= 11", "Firefox ESR"],
},
),
require(
"postcss-url",
)({
url: (
url,
) => url.startsWith("/") || /^[a-z]+:/.test(url) ? url : `/static/${url}`,
},),
],
processors:
[
require(
"autoprefixer",
{
browsers: ["> 1%", "last 2 versions", "ie >= 11", "Firefox ESR"],
},
),
require(
"postcss-url",
)({
url: (
url,
) =>
url.startsWith("/") || /^[a-z]+:/.test(url) ? url : `/static/${url}`,
},),
],
});
true ? test({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,21 +78,22 @@ a(
exports.examples =
[
{
render: withGraphQLQuery(
"node(1234567890){image{uri}}",
function (container, data) {
return (
<div>
render:
withGraphQLQuery(
"node(1234567890){image{uri}}",
function (container, data) {
return (
<div>
<InlineBlock>
<img
src={data[1234567890].image.uri}
style={{position: 'absolute', top: '0', left: '0', zIndex:'-1'}}
/>
</InlineBlock>
</div>
);
},
),
);
},
),
},
];
Expand Down Expand Up @@ -136,6 +137,6 @@ someReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReal
```
2: SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong,
5: SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong: 1,
31: someReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReally.a([
32: someReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReally.a([
```

Loading

0 comments on commit 2d8663c

Please sign in to comment.