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

fix(rome_js_analyze): improve trivia handling in useSingleVarDeclarator #2955

Merged
merged 3 commits into from
Jul 29, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Jul 28, 2022

Summary

This PR fixes #2945 by improving how trivia is handled as tokens are moved in the syntax tree mutation. Since that rule isn't really safe to apply I've downgraded its applicability to MaybeIncorrect, and modified the CLI tests that were relying on this

I also fixed the issue of separated lists created with the node factory API having an empty slot at the end of the list if the trailing separator was None by having the factory function take two iterators (one for the list elements and one for the separators) instead of a single iterator of pairs of list element + an optional separator token. The content of the two iterators are interleaved while building the list, and the separator iterator can simply be one item shorter than the elements iterator in order to omit the last separator without creating an empty slot

Test Plan

I've added the problematic cases from #2945 to the tests for the useSingleVarDeclarator rule, the code actions being emitted for these are now correct

@leops leops requested a review from xunilrj as a code owner July 28, 2022 16:01
@leops leops requested a review from a team July 28, 2022 16:01
@leops leops temporarily deployed to aws July 28, 2022 16:01 Inactive
@github-actions
Copy link

github-actions bot commented Jul 28, 2022

@github-actions
Copy link

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 395 395 0
Failed 5551 5551 0
Panics 0 0 0
Coverage 6.64% 6.64% 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 12393 12393 0
Failed 3864 3864 0
Panics 0 0 0
Coverage 76.23% 76.23% 0.00%

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 29, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: e93b052
Status:⚡️  Build in progress...

View logs

@leops leops temporarily deployed to aws July 29, 2022 09:34 Inactive
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.

This is surely a lot of logic for handling some trivias, and I am sure that we will face some complications moving forward. We should come up with a standard solution at some point

@leops leops temporarily deployed to aws July 29, 2022 16:03 Inactive
@leops leops merged commit 900e437 into main Jul 29, 2022
@leops leops deleted the fix/use-single-var-declarator branch July 29, 2022 16:10
IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022
…or (rome#2955)

* fix(rome_js_analyze): improve trivia handling in useSingleVarDeclarator

* update documentation

* PR review
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.

🐛 Misaligned and wrong code fix for js/useSingleVarDeclarator
2 participants