fix(rome_js_formatter): don't add spaces to comments #2515

ematipico · 2022-04-28T15:22:13Z

Summary

No big changes in this one, I had to make the iterator a peekable as it needs to check if there's a trailing trivia and keep the whitespace in that case.

Test Plan

Added two test cases that cover the cases we want to fix

ematipico · 2022-04-28T15:24:35Z

crates/rome_js_formatter/src/formatter.rs

+                let next_piece = pieces.peek();
+                let leading_trivia = if let Some(previous_piece) = previous_piece {
+                    if previous_piece.is_whitespace() {
+                        space_token()


I am thinking that maybe we can do another optimisation here, where we create a new token out of the trivia, by keeping the same text.

It would an optimization similar to what Micha did recently. This would work only for comments, as they are the only ones that contain actual strings.

github-actions · 2022-04-28T15:24:52Z

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%

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-04-28T15:26:06Z

Deploying with Cloudflare Pages

Latest commit:	`8c01cb4`
Status:	✅ Deploy successful!
Preview URL:	https://586b3e9a.tools-8rn.pages.dev

View logs

github-actions · 2022-04-28T15:27:39Z

Playground for commit 1d07d6a

crates/rome_js_formatter/src/formatter.rs

yassere · 2022-04-28T17:29:26Z

!bench_formatter

crates/rome_js_formatter/src/lib.rs

github-actions · 2022-04-28T17:58:24Z

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00    398.3±1.56ms     6.5 MB/sec    1.00    396.3±1.84ms     6.6 MB/sec
formatter/compiler.js                    1.01    245.2±1.14ms     4.3 MB/sec    1.00    243.1±1.43ms     4.3 MB/sec
formatter/d3.min.js                      1.00    187.6±1.41ms  1430.4 KB/sec    1.00    186.8±1.25ms  1436.8 KB/sec
formatter/dojo.js                        1.00     13.3±0.03ms     5.1 MB/sec    1.00     13.4±0.07ms     5.1 MB/sec
formatter/ios.d.ts                       1.00    290.3±1.07ms     6.4 MB/sec    1.00    290.5±1.01ms     6.4 MB/sec
formatter/jquery.min.js                  1.00     52.8±0.25ms  1601.4 KB/sec    1.00     52.6±0.14ms  1608.1 KB/sec
formatter/math.js                        1.01    378.6±1.23ms  1751.5 KB/sec    1.00    374.6±1.91ms  1769.9 KB/sec
formatter/parser.ts                      1.01      9.4±0.05ms     5.1 MB/sec    1.00      9.3±0.01ms     5.2 MB/sec
formatter/pixi.min.js                    1.00    206.6±0.97ms     2.1 MB/sec    1.00    207.1±1.42ms     2.1 MB/sec
formatter/react-dom.production.min.js    1.01     63.1±0.50ms  1866.7 KB/sec    1.00     62.3±0.20ms  1892.5 KB/sec
formatter/react.production.min.js        1.00      3.1±0.01ms  2026.8 KB/sec    1.00      3.1±0.01ms  2033.7 KB/sec
formatter/router.ts                      1.01      7.2±0.03ms     8.4 MB/sec    1.00      7.2±0.03ms     8.5 MB/sec
formatter/tex-chtml-full.js              1.00    469.1±1.26ms  1989.2 KB/sec    1.00    468.1±3.01ms  1993.7 KB/sec
formatter/three.min.js                   1.00    238.5±0.98ms     2.5 MB/sec    1.01    240.6±0.97ms     2.4 MB/sec
formatter/typescript.js                  1.01   1570.1±3.84ms     6.1 MB/sec    1.00   1556.9±3.51ms     6.1 MB/sec
formatter/vue.global.prod.js             1.00     82.9±0.54ms  1487.5 KB/sec    1.00     83.3±0.50ms  1481.7 KB/sec

NicholasLYang · 2022-04-28T18:51:05Z

crates/rome_js_formatter/src/formatter.rs

+
+        let mut pieces = pieces.peekable();
+
+        while let Some(piece) = pieces.next() {


If I'm reading this correctly, we should be checking for the pieces that are not comments to see if they are whitespace and changing has_seen_whitespace to true, no? Because currently we do not print a space when there's a line of code followed by a space then a single line comment: Playground

NicholasLYang · 2022-04-28T18:54:04Z

crates/rome_js_formatter/src/formatter.rs

+                    empty_element()
+                };
+
+                let trailing_trivia = if next_piece.map_or(false, SyntaxTriviaPiece::is_whitespace)


We don't always want to print a space if the next trivia piece is whitespace. For instance if we have foo( /* bar */ ), this should be formatted as foo(/* bar */): Playground

I hoped this could have been consistent but it seems that the spacing of the comments depends on where they are placed.. meaning in which nodes they are.

Yes, the formatting is specific to what the previous/next token is. See the related issue, I outlined the rules.

I'm currently looking into how we should format comments. I do like this implementation but believe that we should instead test if the next/previous token is a punctuation token and, if so, not insert a space. I haven't yet figured out how to do this best in here with skipped token trivia but I'm honestly inclined to just bail out formatting for skipped token trivia

MichaReiser · 2022-05-16T13:10:40Z

Prettier categories comments as:

Leading: In front of a node `class /* in front of identifier Test */ Test {}
Trailing: After a node, but not followed by a whitespace and only if it is the first comment: class Test {} /* Trailing for class node */
Dangling: Between two tokens: function test(/*no arguments*/) {}

And Prettier has an extensive rules on when it diverges from these simple rules

https://github.com/prettier/prettier/blob/1dffa0f9dac571bf0e03969c11fd4548efd44d7a/src/language-js/comments.js#L1-L1036

MichaReiser · 2022-06-13T13:19:26Z

I'm closing this PR because #2701 implements the spacing before/after inline comments based on the previous/next token.

ematipico added 3 commits April 28, 2022 15:28

fix(rome_js_formatter): keep prefix trivia

64b1ea1

fix(rome_js_formatter): keep leading trivia in single line comments

160f3fa

fix(rome_js_formatter): keep trailing trivia in single line comments

1240b84

ematipico temporarily deployed to aws April 28, 2022 15:22 Inactive

ematipico requested review from leops, NicholasLYang and yassere April 28, 2022 15:23

ematipico commented Apr 28, 2022

View reviewed changes

leops reviewed Apr 28, 2022

View reviewed changes

crates/rome_js_formatter/src/formatter.rs Outdated Show resolved Hide resolved

crates/rome_js_formatter/src/formatter.rs Outdated Show resolved Hide resolved

chore: code review

8c01cb4

ematipico requested a review from leops April 28, 2022 16:34

ematipico temporarily deployed to aws April 28, 2022 16:34 Inactive

yassere reviewed Apr 28, 2022

View reviewed changes

crates/rome_js_formatter/src/lib.rs Show resolved Hide resolved

NicholasLYang suggested changes Apr 28, 2022

View reviewed changes

MichaReiser closed this Jun 13, 2022

ematipico deleted the fix/comment-spacing branch June 13, 2022 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rome_js_formatter): don't add spaces to comments #2515

fix(rome_js_formatter): don't add spaces to comments #2515

ematipico commented Apr 28, 2022

ematipico Apr 28, 2022

github-actions bot commented Apr 28, 2022

cloudflare-workers-and-pages bot commented Apr 28, 2022 •

edited

Loading

github-actions bot commented Apr 28, 2022 •

edited

Loading

yassere commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

NicholasLYang Apr 28, 2022

NicholasLYang Apr 28, 2022

ematipico Apr 29, 2022

MichaReiser May 2, 2022

MichaReiser May 21, 2022

MichaReiser commented May 16, 2022

MichaReiser commented Jun 13, 2022


		let mut pieces = pieces.peekable();

		while let Some(piece) = pieces.next() {

fix(rome_js_formatter): don't add spaces to comments #2515

fix(rome_js_formatter): don't add spaces to comments #2515

Conversation

ematipico commented Apr 28, 2022

Summary

Test Plan

ematipico Apr 28, 2022

Choose a reason for hiding this comment

github-actions bot commented Apr 28, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

ts/babel

ts/microsoft

cloudflare-workers-and-pages bot commented Apr 28, 2022 • edited Loading

Deploying with Cloudflare Pages

github-actions bot commented Apr 28, 2022 • edited Loading

yassere commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

Formatter Benchmark Results

NicholasLYang Apr 28, 2022

Choose a reason for hiding this comment

NicholasLYang Apr 28, 2022

Choose a reason for hiding this comment

ematipico Apr 29, 2022

Choose a reason for hiding this comment

MichaReiser May 2, 2022

Choose a reason for hiding this comment

MichaReiser May 21, 2022

Choose a reason for hiding this comment

MichaReiser commented May 16, 2022

MichaReiser commented Jun 13, 2022

cloudflare-workers-and-pages bot commented Apr 28, 2022 •

edited

Loading

github-actions bot commented Apr 28, 2022 •

edited

Loading