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

fix(rome_js_formatter): 🐛 preserve new-lines after directives #2500

Merged
merged 4 commits into from
Apr 27, 2022

Conversation

IWANABETHATGUY
Copy link
Contributor

@IWANABETHATGUY IWANABETHATGUY commented Apr 27, 2022

Summary

Fix #2437

Test Plan

The updated snapshot should fix related issue #2437,
you could review the snapshot this pr updated.

@IWANABETHATGUY
Copy link
Contributor Author

!bench_formatter

@github-actions
Copy link

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00    379.5±1.33ms     6.8 MB/sec    1.02    388.8±1.79ms     6.7 MB/sec
formatter/compiler.js                    1.00    230.8±0.86ms     4.5 MB/sec    1.00    231.0±1.79ms     4.5 MB/sec
formatter/d3.min.js                      1.00    179.1±0.77ms  1498.4 KB/sec    1.00    179.6±0.71ms  1494.4 KB/sec
formatter/dojo.js                        1.00     12.3±0.01ms     5.6 MB/sec    1.00     12.4±0.01ms     5.5 MB/sec
formatter/ios.d.ts                       1.00    277.8±0.52ms     6.7 MB/sec    1.02    282.8±0.58ms     6.6 MB/sec
formatter/jquery.min.js                  1.00     47.2±0.37ms  1794.3 KB/sec    1.00     47.2±0.34ms  1791.5 KB/sec
formatter/math.js                        1.00    362.4±1.17ms  1829.7 KB/sec    1.00    362.9±1.10ms  1826.9 KB/sec
formatter/parser.ts                      1.00      8.6±0.01ms     5.6 MB/sec    1.01      8.6±0.01ms     5.6 MB/sec
formatter/pixi.min.js                    1.00    202.0±0.88ms     2.2 MB/sec    1.00    202.0±0.91ms     2.2 MB/sec
formatter/react-dom.production.min.js    1.00     60.1±0.61ms  1960.7 KB/sec    1.00     59.8±0.77ms  1969.8 KB/sec
formatter/react.production.min.js        1.00      2.9±0.00ms     2.1 MB/sec    1.00      2.9±0.00ms     2.1 MB/sec
formatter/router.ts                      1.00      6.3±0.00ms     9.7 MB/sec    1.01      6.4±0.00ms     9.6 MB/sec
formatter/tex-chtml-full.js              1.00    449.1±1.00ms     2.0 MB/sec    1.00    449.4±1.00ms     2.0 MB/sec
formatter/three.min.js                   1.00    229.8±1.03ms     2.6 MB/sec    1.00    230.2±0.87ms     2.6 MB/sec
formatter/typescript.js                  1.00  1458.5±10.01ms     6.5 MB/sec    1.02   1481.6±3.47ms     6.4 MB/sec
formatter/vue.global.prod.js             1.00     79.3±0.93ms  1555.3 KB/sec    1.00     79.3±0.86ms  1555.0 KB/sec

@IWANABETHATGUY
Copy link
Contributor Author

@ematipico
Copy link
Contributor

It seems that I became a core contributor,
https://github.com/rome/tools/blob/main/CONTRIBUTING.md#if-you-are-a-core-contributor

I think it was a mistake of mine. I didn't know contributors could run bench tests, which is great!

Although compliance tests can't be, that one I know 🤣

@ematipico ematipico changed the title fix: 🐛 preserve new-lines after directives fix(rome_js_formatter): 🐛 preserve new-lines after directives Apr 27, 2022
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

The end result is what we needed! I would suggest the following change.

We already have a logic that checks the presence of lines between nodes:

https://github.com/rome/tools/blob/main/crates/rome_formatter/src/format_element.rs#L828-L844

My suggestion is to make this function public and use this one instead. The reason why we should factor out this function is because it's becoming a recurrent pattern that is used in multiple cases.

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@ematipico ematipico merged commit 97fc6b7 into rome:main Apr 27, 2022
@IWANABETHATGUY IWANABETHATGUY deleted the fix/new-lines-directives branch April 29, 2022 05:24
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.

Preserve new-lines after directives
2 participants