close
Skip to content

challenge(formatter): Attach comments to labels#730

Merged
ematipico merged 1 commit intomainfrom
conaclos/challenge/js/comments/break-continue-statements.js
Nov 18, 2023
Merged

challenge(formatter): Attach comments to labels#730
ematipico merged 1 commit intomainfrom
conaclos/challenge/js/comments/break-continue-statements.js

Conversation

@Conaclos
Copy link
Copy Markdown
Member

@Conaclos Conaclos commented Nov 14, 2023

Summary

This partially fixes js/comments/break-continue-statements.js.

Prettier has a dedicated syntax node for labels. This allows attaching comments to the label. Biome has n osuch node and thus moves comments at the end of the statement.

To fix the difference, I introduced a new CST node JsLabel.
The node have to be introduced for JsBreakStatement and JsContinueSTatement. For uniformity, I also used JsLabel for JsLabeledSatteement. Alternatively, we could still directly use a token for JsLabeledSatteement or differentiate JsLabel into JsLabelReference and JsLabelBinding. Any opinion?

For now, leading comments are correctly attached to the label.
I encounter an issue with trailing comments: when I changed the code to attach them to the label I got a reformat instability.

Test Plan

Updated.

@Conaclos Conaclos temporarily deployed to Website deployment November 14, 2023 22:24 — with BERJAYA GitHub Actions Inactive
@github-actions github-actions Bot added A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages labels Nov 14, 2023
@github-actions
Copy link
Copy Markdown
Contributor

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 49701 49701 0
Passed 48721 48721 0
Failed 980 980 0
Panics 0 0 0
Coverage 98.03% 98.03% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6322 6322 0
Passed 2036 2036 0
Failed 4286 4286 0
Panics 0 0 0
Coverage 32.20% 32.20% 0.00%

ts/babel

Test result main count This PR count Difference
Total 662 662 0
Passed 592 592 0
Failed 70 70 0
Panics 0 0 0
Coverage 89.43% 89.43% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17646 17646 0
Passed 13454 13454 0
Failed 4190 4190 0
Panics 2 2 0
Coverage 76.24% 76.24% 0.00%

@Conaclos Conaclos force-pushed the conaclos/challenge/js/comments/break-continue-statements.js branch from 730c909 to d35c1a5 Compare November 14, 2023 23:30
@Conaclos Conaclos temporarily deployed to Website deployment November 14, 2023 23:30 — with BERJAYA GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/challenge/js/comments/break-continue-statements.js branch from d35c1a5 to 1de9074 Compare November 17, 2023 13:33
@Conaclos Conaclos temporarily deployed to Website deployment November 17, 2023 13:33 — with BERJAYA GitHub Actions Inactive
@Conaclos Conaclos force-pushed the conaclos/challenge/js/comments/break-continue-statements.js branch from 1de9074 to 76023c1 Compare November 17, 2023 13:36
@ematipico
Copy link
Copy Markdown
Member

It's a bit unfortunate that we need to add a new node. But I suppose it's fine!

@ematipico ematipico merged commit 3b0659a into main Nov 18, 2023
@ematipico ematipico deleted the conaclos/challenge/js/comments/break-continue-statements.js branch November 18, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Formatter Area: formatter A-Parser Area: parser A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants