feat(rome_js_formatter): elide quotes in object/class members #2536

ematipico · 2022-05-04T10:17:50Z

Summary

This PR closes #2405

I cherry-picked the commits of the previous PR that introduced these changes in the first place, then applied the logic for checking the source type.

Test Plan

I created new test cases.

Unfortunately it's not easy to compare it with Prettier output, that's why I suggest to compare their playground with our snapshots.

ematipico · 2022-05-04T10:19:06Z

...s/rome_js_formatter/tests/specs/prettier/typescript/custom/computedProperties/string.ts.snap

 }

-type B = { "string": "B" };
+type B = { string: "B" };


This matches prettier's output

github-actions · 2022-05-04T10:20:23Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45878	45878	0
Passed	44938	44938	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	`main` count	This PR count	Difference
Total	5946	5946	0
Passed	350	350	0
Failed	5590	5591	❌ ⏫ +1
Panics	6	5	✅ ⏬ -1
Coverage	5.89%	5.89%	0.00%

⁉️ Panic To Failed (1):

typeGuardsAsAssertions.symbols

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12391	12391	0
Failed	3866	3866	0
Panics	0	0	0
Coverage	76.22%	76.22%	0.00%

cloudflare-workers-and-pages · 2022-05-04T10:20:43Z

Deploying with Cloudflare Pages

Latest commit:	`125ad84`
Status:	✅ Deploy successful!
Preview URL:	https://a0ddff2a.tools-8rn.pages.dev

View logs

github-actions · 2022-05-04T10:22:36Z

Playground for commit b6e109f

crates/rome_js_formatter/src/utils/mod.rs

MichaReiser

It turns out that prettier applies the same logic to both JS and TS.

This isn't the case for all number

crates/rome_js_formatter/src/utils/mod.rs

MichaReiser · 2022-05-05T13:27:16Z

crates/rome_js_formatter/src/utils/mod.rs

+        text_to_check.chars().enumerate().all(|(index, c)| {
+            // Members that starts with numbers in their name are not valid syntax,
+            // hence we can't elide the quotes
+            if index == 0 && c.is_numeric() {


I just checked this again and we forgot that we're talking about members, not identifiers.

A all digits string member can be turned into a number literal that.

{ "123": true, // same as 123: true }

However, Prettier doesn't unquote all number members in TypeScript.

So the rule must support:

string forming a valid identifier -> print as identifier

string forming a valid number -> print as number literal (only for JS)

That is true, but prettier doesn't seem to remove the quotes from numeric-like members (that can be valid members): playground

You need to change the parser to babel.

This should be flagged now, I added a new test that should cover the case

ematipico · 2022-05-05T14:55:15Z

This isn't the case for all number

Could you please share some cases that I might have missed, please? I couldn't find one

MichaReiser · 2022-05-06T06:23:01Z

This isn't the case for all number

Could you please share some cases that I might have missed, please? I couldn't find one

Sorry, I didn't intend to submit that comment. I first started this comment before I then wrote the more detailed comment in utils/mod.rs

crates/rome_js_formatter/src/utils/mod.rs

MichaReiser · 2022-05-06T06:30:30Z

crates/rome_js_formatter/src/utils/mod.rs

+        text_to_check.chars().enumerate().all(|(index, c)| {
+            // Members that starts with numbers in their name are not valid syntax,
+            // hence we can't elide the quotes
+            if index == 0 && c.is_numeric() {


You need to change the parser to babel.

ematipico · 2022-05-06T07:32:20Z

PR on hold as at the moment the formatter doesn't have the understanding if it's parsing a TS file or a JS file.

In order to give this information to the formatter, we have to extract it from the SourceType. We will need to pass this information first, and then we might be able to achieve this.

MichaReiser

I'll need to review the rest tomorrow. This is some sophisticated stuff that my brain isn't capable of handling at this time of a day :D

crates/rome_js_formatter/src/utils/mod.rs

crates/rome_js_formatter/src/utils/string_utils.rs

MichaReiser

Looks good. One thing that I'm wondering is if it would be better to have small helper functions for the string cleaning but then have different

FormatLiteralMemberName
FormatDirectiveLiteral
...

Functions that call into this helpers. It could help to remove some branching and may help to split this code into some smaller files.

I'm considering this because it feels like it's becoming this one string manipulation that is able to handle ALL cases which makes it harder to reason about how the code works (for a specific case)

crates/rome_js_formatter/src/js/assignments/object_assignment_pattern_property.rs

crates/rome_js_formatter/src/js/bindings/object_binding_pattern_property.rs

crates/rome_js_formatter/src/js/classes/constructor_class_member.rs

crates/rome_js_formatter/src/js/classes/getter_class_member.rs

crates/rome_js_formatter/src/js/classes/method_class_member.rs

crates/rome_js_formatter/src/utils/mod.rs

crates/rome_js_formatter/src/utils/string_utils.rs

ematipico · 2022-06-03T11:30:51Z

Looks good. One thing that I'm wondering is if it would be better to have small helper functions for the string cleaning but then have different
* `FormatLiteralMemberName`

* `FormatDirectiveLiteral`

* ...
Functions that call into this helpers. It could help to remove some branching and may help to split this code into some smaller files.

I'm considering this because it feels like it's becoming this one string manipulation that is able to handle ALL cases which makes it harder to reason about how the code works (for a specific case)

Makes sense. Allow me to address these concerns in the next PR. The works on these PR are not finished yet. The cases are not over unfortunately (we still have the escaping of other chars) . At this point it might make more sense to also have a small module for each case (directive, member, literal string)

MichaReiser · 2022-06-03T11:52:45Z

Looks good. One thing that I'm wondering is if it would be better to have small helper functions for the string cleaning but then have different
* `FormatLiteralMemberName`

* `FormatDirectiveLiteral`

* ...
Functions that call into this helpers. It could help to remove some branching and may help to split this code into some smaller files.
I'm considering this because it feels like it's becoming this one string manipulation that is able to handle ALL cases which makes it harder to reason about how the code works (for a specific case)
Makes sense. Allow me to address these concerns in the next PR. The works on these PR are not finished yet. The cases are not over unfortunately (we still have the escaping of other chars) . At this point it might make more sense to also have a small module for each case (directive, member, literal string)

I'm not opposed to it. Is this something you're planning to work on soon?

ematipico · 2022-06-03T14:28:41Z

I'm not opposed to it. Is this something you're planning to work on soon?

Yes, it's in my bucket list :)

ematipico temporarily deployed to aws May 4, 2022 10:17 Inactive

ematipico commented May 4, 2022

View reviewed changes

IWANABETHATGUY reviewed May 5, 2022

View reviewed changes

crates/rome_js_formatter/src/utils/mod.rs Outdated Show resolved Hide resolved

ematipico temporarily deployed to aws May 5, 2022 12:33 Inactive

ematipico requested review from leops, NicholasLYang and yassere May 5, 2022 12:33

MichaReiser reviewed May 5, 2022

View reviewed changes

ematipico temporarily deployed to aws May 5, 2022 15:18 Inactive

ematipico requested a review from MichaReiser May 5, 2022 15:19

MichaReiser suggested changes May 6, 2022

View reviewed changes

ematipico added the PR: on hold A PR that needs some upstream work before getting merged. label May 6, 2022

ematipico mentioned this pull request May 6, 2022

Make the formatter language aware #2546

Closed

ematipico added 2 commits May 31, 2022 14:16

fix(rome_js_formatter): keep quotes for numeric members

6fee04c

chore: wip

a77bac5

ematipico force-pushed the fix/keep-quotes-on-numbers branch from ee6d28e to 9872483 Compare May 31, 2022 16:42

ematipico temporarily deployed to aws May 31, 2022 16:43 Inactive

ematipico changed the title ~~fix(rome_js_formatter): keep quotes for numeric members~~ feat(rome_js_formatter): elide quotes in object/class members May 31, 2022

ematipico removed the PR: on hold A PR that needs some upstream work before getting merged. label May 31, 2022

ematipico force-pushed the fix/keep-quotes-on-numbers branch from 9872483 to 250a139 Compare May 31, 2022 16:54

ematipico temporarily deployed to aws May 31, 2022 16:54 Inactive

feat(rome_js_formatter): elide quotes in object properties

a79ceef

ematipico force-pushed the fix/keep-quotes-on-numbers branch from 250a139 to a79ceef Compare May 31, 2022 16:58

ematipico temporarily deployed to aws May 31, 2022 16:58 Inactive

ematipico requested a review from MichaReiser May 31, 2022 16:59

MichaReiser reviewed May 31, 2022

View reviewed changes

crates/rome_js_formatter/src/utils/mod.rs Outdated Show resolved Hide resolved

crates/rome_js_formatter/src/utils/string_utils.rs Show resolved Hide resolved

crates/rome_js_formatter/src/utils/string_utils.rs Outdated Show resolved Hide resolved

ematipico added 2 commits June 1, 2022 12:21

chore: implement Format for new enum

44ec2e0

docs: explain new struct

4701fa8

ematipico temporarily deployed to aws June 1, 2022 14:44 Inactive

ematipico requested a review from MichaReiser June 1, 2022 14:44

MichaReiser reviewed Jun 2, 2022

View reviewed changes

ematipico temporarily deployed to aws June 3, 2022 11:24 Inactive

chore: address code review

125ad84

ematipico force-pushed the fix/keep-quotes-on-numbers branch from 74dc656 to 125ad84 Compare June 3, 2022 11:29

ematipico requested a review from MichaReiser June 3, 2022 11:29

ematipico temporarily deployed to aws June 3, 2022 11:29 Inactive

MichaReiser approved these changes Jun 3, 2022

View reviewed changes

ematipico merged commit d46fcfd into main Jun 3, 2022

ematipico deleted the fix/keep-quotes-on-numbers branch June 3, 2022 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_formatter): elide quotes in object/class members #2536

feat(rome_js_formatter): elide quotes in object/class members #2536

ematipico commented May 4, 2022 •

edited

Loading

ematipico May 4, 2022

github-actions bot commented May 4, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented May 4, 2022 •

edited

Loading

github-actions bot commented May 4, 2022 •

edited

Loading

MichaReiser left a comment

MichaReiser May 5, 2022

ematipico May 5, 2022 •

edited

Loading

MichaReiser May 6, 2022

ematipico May 31, 2022

ematipico commented May 5, 2022 •

edited

Loading

MichaReiser commented May 6, 2022

MichaReiser May 6, 2022

ematipico commented May 6, 2022

MichaReiser left a comment

MichaReiser left a comment

ematipico commented Jun 3, 2022 •

edited

Loading

MichaReiser commented Jun 3, 2022

ematipico commented Jun 3, 2022

feat(rome_js_formatter): elide quotes in object/class members #2536

feat(rome_js_formatter): elide quotes in object/class members #2536

Conversation

ematipico commented May 4, 2022 • edited Loading

Summary

Test Plan

ematipico May 4, 2022

Choose a reason for hiding this comment

github-actions bot commented May 4, 2022 • edited Loading

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

cloudflare-workers-and-pages bot commented May 4, 2022 • edited Loading

Deploying with Cloudflare Pages

github-actions bot commented May 4, 2022 • edited Loading

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser May 5, 2022

Choose a reason for hiding this comment

ematipico May 5, 2022 • edited Loading

Choose a reason for hiding this comment

MichaReiser May 6, 2022

Choose a reason for hiding this comment

ematipico May 31, 2022

Choose a reason for hiding this comment

ematipico commented May 5, 2022 • edited Loading

MichaReiser commented May 6, 2022

MichaReiser May 6, 2022

Choose a reason for hiding this comment

ematipico commented May 6, 2022

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

ematipico commented Jun 3, 2022 • edited Loading

MichaReiser commented Jun 3, 2022

ematipico commented Jun 3, 2022

ematipico commented May 4, 2022 •

edited

Loading

github-actions bot commented May 4, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented May 4, 2022 •

edited

Loading

github-actions bot commented May 4, 2022 •

edited

Loading

ematipico May 5, 2022 •

edited

Loading

ematipico commented May 5, 2022 •

edited

Loading

ematipico commented Jun 3, 2022 •

edited

Loading