Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_formatter): custom formatting for test calls #2769

Merged
merged 12 commits into from
Jun 24, 2022

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Jun 23, 2022

Summary

This PR implements gets us closer to how call arguments are formatted.

Prettier has a particular formatting just for call expressions that are used by testing frameworks, such as

describe("Component X", () => {});
it.only("it does want I want", t => {});

This PR brings their algorithm into our code base and it uses it to do an early formatting if the criteria are met. The function explains what the criteria are.

While doing so, I added an option to JsCallArguments to accept an option called callee, which is essentially the callee that belong to its parent. The reason why I added this option is because this check should not be used for call expressions of dynamic import. So instead of calling the parent, checking its type and then extracting thecallee, I decided to pass a reference of the callee from the parent.

It addresses a comment from closed PR, by limiting the usage of a custom buffer to a single call, instead of dragging the whole buffer across functions;

Test Plan

cargo test and made sure that the prettier testing suite format as prettier's

PR

File Based Average Prettier Similarity: 76.09%
Line Based Average Prettier Similarity: 71.53%

main
File Based Average Prettier Similarity: 76.00%
Line Based Average Prettier Similarity: 70.84%

@ematipico ematipico temporarily deployed to aws June 23, 2022 14:38 Inactive
@github-actions
Copy link

github-actions bot commented Jun 23, 2022

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jun 23, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6f00389
Status: ✅  Deploy successful!
Preview URL: https://c839bbbb.tools-8rn.pages.dev
Branch Preview URL: https://feature-will-break-functiona.tools-8rn.pages.dev

View logs

@ematipico ematipico temporarily deployed to aws June 23, 2022 14:43 Inactive
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I think it would be good to split out the WillBreakBuffer into its own PR as the other logic in this PR isn't using it.

crates/rome_formatter/src/builders.rs Outdated Show resolved Hide resolved
@ematipico ematipico temporarily deployed to aws June 23, 2022 15:25 Inactive
@ematipico ematipico temporarily deployed to aws June 23, 2022 15:53 Inactive
@ematipico ematipico temporarily deployed to aws June 23, 2022 15:58 Inactive
@ematipico ematipico force-pushed the feature/will-break-functionality branch from 4e6c0a3 to c96d02f Compare June 23, 2022 15:59
@ematipico ematipico temporarily deployed to aws June 23, 2022 15:59 Inactive
@ematipico
Copy link
Contributor Author

I think it would be good to split out the WillBreakBuffer into its own PR as the other logic in this PR isn't using it.

Done, that's been removed

@MichaReiser
Copy link
Contributor

!bench_formatter

@github-actions
Copy link

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00   499.5±18.00ms     5.2 MB/sec    1.02   510.1±18.02ms     5.1 MB/sec
formatter/compiler.js                    1.00    318.8±8.74ms     3.3 MB/sec    1.02   325.4±12.62ms     3.2 MB/sec
formatter/d3.min.js                      1.00    268.1±7.66ms  1001.0 KB/sec    1.02   273.7±10.56ms   980.7 KB/sec
formatter/dojo.js                        1.00     16.3±0.82ms     4.2 MB/sec    1.08     17.7±1.14ms     3.9 MB/sec
formatter/ios.d.ts                       1.00   336.6±13.03ms     5.5 MB/sec    1.01   338.9±13.58ms     5.5 MB/sec
formatter/jquery.min.js                  1.00     70.1±3.58ms  1207.1 KB/sec    1.00     70.2±4.03ms  1204.9 KB/sec
formatter/math.js                        1.04   539.1±10.09ms  1230.0 KB/sec    1.00   518.8±16.94ms  1278.1 KB/sec
formatter/parser.ts                      1.00     10.9±0.43ms     4.4 MB/sec    1.06     11.6±0.71ms     4.2 MB/sec
formatter/pixi.min.js                    1.00   307.0±13.31ms  1463.8 KB/sec    1.00   308.2±11.87ms  1458.4 KB/sec
formatter/react-dom.production.min.js    1.00     85.8±4.41ms  1373.9 KB/sec    1.03     88.5±4.61ms  1332.5 KB/sec
formatter/react.production.min.js        1.00      4.0±0.17ms  1565.4 KB/sec    1.01      4.0±0.27ms  1556.1 KB/sec
formatter/router.ts                      1.00      7.8±0.26ms     7.5 MB/sec    1.06      8.3±0.36ms     7.1 MB/sec
formatter/tex-chtml-full.js              1.05   700.5±15.36ms  1332.1 KB/sec    1.00   668.9±21.71ms  1395.0 KB/sec
formatter/three.min.js                   1.01   350.7±14.10ms  1714.2 KB/sec    1.00   347.6±15.13ms  1729.3 KB/sec
formatter/typescript.js                  1.00  1999.8±43.46ms     4.8 MB/sec    1.01       2.0±0.06s     4.7 MB/sec
formatter/vue.global.prod.js             1.04    114.2±6.13ms  1080.4 KB/sec    1.00    109.8±5.96ms  1123.3 KB/sec

@ematipico ematipico temporarily deployed to aws June 23, 2022 16:49 Inactive
@ematipico ematipico temporarily deployed to aws June 23, 2022 17:55 Inactive
@ematipico
Copy link
Contributor Author

ematipico commented Jun 23, 2022

I pushed a small refactor and it uses only references, and a better way to do checks on the arguments for the custom formatting (hooks and tests). It should improve slightly the performances.

@ematipico ematipico force-pushed the feature/will-break-functionality branch from 063e4d3 to 8a0c3af Compare June 23, 2022 18:04
@ematipico ematipico temporarily deployed to aws June 23, 2022 18:04 Inactive
@ematipico ematipico force-pushed the feature/will-break-functionality branch from 8a0c3af to 568aca5 Compare June 23, 2022 18:05
@ematipico ematipico temporarily deployed to aws June 23, 2022 18:05 Inactive
@ematipico
Copy link
Contributor Author

!bench_formatter

@github-actions
Copy link

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00   384.3±16.12ms     6.8 MB/sec    1.00   383.8±16.48ms     6.8 MB/sec
formatter/compiler.js                    1.00   242.5±10.33ms     4.3 MB/sec    1.01   245.8±10.99ms     4.3 MB/sec
formatter/d3.min.js                      1.00    202.9±8.85ms  1323.0 KB/sec    1.05   212.2±12.35ms  1264.6 KB/sec
formatter/dojo.js                        1.00     13.4±0.47ms     5.1 MB/sec    1.12     14.9±0.36ms     4.6 MB/sec
formatter/ios.d.ts                       1.07    264.9±9.73ms     7.0 MB/sec    1.00    246.8±9.66ms     7.6 MB/sec
formatter/jquery.min.js                  1.00     55.2±2.01ms  1534.0 KB/sec    1.04     57.3±1.61ms  1476.6 KB/sec
formatter/math.js                        1.00   398.9±13.12ms  1662.4 KB/sec    1.08   430.1±13.76ms  1541.6 KB/sec
formatter/parser.ts                      1.00      9.1±0.33ms     5.3 MB/sec    1.04      9.5±0.25ms     5.1 MB/sec
formatter/pixi.min.js                    1.00    223.0±8.35ms  2015.4 KB/sec    1.03   229.3±11.13ms  1959.6 KB/sec
formatter/react-dom.production.min.js    1.00     61.8±3.27ms  1907.9 KB/sec    1.10     68.2±2.97ms  1728.1 KB/sec
formatter/react.production.min.js        1.00      3.3±0.23ms  1930.9 KB/sec    1.05      3.4±0.07ms  1833.7 KB/sec
formatter/router.ts                      1.00      6.7±0.19ms     8.8 MB/sec    1.00      6.7±0.25ms     8.8 MB/sec
formatter/tex-chtml-full.js              1.00   536.9±27.55ms  1738.2 KB/sec    1.00   534.5±17.90ms  1745.7 KB/sec
formatter/three.min.js                   1.00    258.6±9.85ms     2.3 MB/sec    1.03    266.0±9.34ms     2.2 MB/sec
formatter/typescript.js                  1.00  1586.9±51.88ms     6.0 MB/sec    1.00  1585.5±69.30ms     6.0 MB/sec
formatter/vue.global.prod.js             1.00     92.3±4.06ms  1336.4 KB/sec    1.03     95.4±2.59ms  1293.3 KB/sec

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Looks good. I've one comment around testing the match_test_call where I believe it should be possible to improve the performance by avoiding iterating O(patterns) times overall member names for any non-test method.

@ematipico ematipico force-pushed the feature/will-break-functionality branch from c65a5f9 to b98dc86 Compare June 24, 2022 11:48
@ematipico ematipico temporarily deployed to aws June 24, 2022 11:48 Inactive
@ematipico ematipico temporarily deployed to aws June 24, 2022 13:11 Inactive
@ematipico ematipico force-pushed the feature/will-break-functionality branch from 4380eaa to 6f00389 Compare June 24, 2022 13:31
@ematipico ematipico temporarily deployed to aws June 24, 2022 13:31 Inactive
@ematipico ematipico force-pushed the feature/will-break-functionality branch from 6f00389 to 2f0b4df Compare June 24, 2022 13:32
@ematipico ematipico temporarily deployed to aws June 24, 2022 13:32 Inactive
@ematipico ematipico force-pushed the feature/will-break-functionality branch from 2f0b4df to 5654902 Compare June 24, 2022 13:37
@ematipico ematipico temporarily deployed to aws June 24, 2022 13:37 Inactive
@ematipico ematipico merged commit 4e5eb68 into main Jun 24, 2022
@ematipico ematipico deleted the feature/will-break-functionality branch June 24, 2022 13:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants