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

refactor(rome_js_analyze): groups #3192

Merged
merged 12 commits into from
Sep 13, 2022
Merged

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Sep 9, 2022

Summary

Note: This PR is a highly disruptive. It also changes the documentation of the website. Maybe we should merge before the next release.

Relevant discussion #3017 (comment)

This PR refactor the group names, please read the relevant discussion for more details.

Test Plan

Updated documentation, CLI tests, analyzer tests. The CI should pass because no new functionality has been implemented.

@netlify
Copy link

netlify bot commented Sep 9, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit bb729c4
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/632099cdc85849000a3fdfc4

@ematipico ematipico force-pushed the refactor/rename-linter-groups branch from a862c6c to 8deeeb3 Compare September 9, 2022 13:27
@ematipico ematipico temporarily deployed to netlify-playground September 9, 2022 13:27 Inactive
@github-actions
Copy link

github-actions bot commented Sep 9, 2022

@github-actions
Copy link

github-actions bot commented Sep 9, 2022

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45879 45879 0
Passed 44939 44939 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 1621 1621 0
Failed 4325 4325 0
Panics 0 0 0
Coverage 27.26% 27.26% 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 12395 12395 0
Failed 3862 3862 0
Panics 0 0 0
Coverage 76.24% 76.24% 0.00%

@ematipico ematipico force-pushed the refactor/rename-linter-groups branch from 8deeeb3 to f237fe1 Compare September 12, 2022 08:22
@ematipico ematipico temporarily deployed to netlify-playground September 12, 2022 08:22 Inactive
@ematipico ematipico added the A-Linter Area: linter label Sep 12, 2022
@ematipico ematipico added this to the 0.10.0 milestone Sep 12, 2022
@ematipico ematipico temporarily deployed to netlify-playground September 12, 2022 08:29 Inactive
@ematipico ematipico temporarily deployed to netlify-playground September 12, 2022 08:33 Inactive
@ematipico ematipico force-pushed the refactor/rename-linter-groups branch from 98b0741 to de4227a Compare September 12, 2022 09:11
@ematipico ematipico temporarily deployed to netlify-playground September 12, 2022 09:11 Inactive
@ematipico ematipico force-pushed the refactor/rename-linter-groups branch from de4227a to 28d2b7d Compare September 12, 2022 09:11
@ematipico ematipico temporarily deployed to netlify-playground September 12, 2022 09:11 Inactive
@ematipico ematipico force-pushed the refactor/rename-linter-groups branch from 28d2b7d to d8bd27d Compare September 12, 2022 09:13
@ematipico ematipico temporarily deployed to netlify-playground September 12, 2022 09:13 Inactive
@ematipico ematipico force-pushed the refactor/rename-linter-groups branch from d8bd27d to 74508a0 Compare September 12, 2022 09:15
@ematipico ematipico temporarily deployed to netlify-playground September 12, 2022 09:15 Inactive
@ematipico ematipico temporarily deployed to netlify-playground September 12, 2022 09:27 Inactive
@ematipico ematipico marked this pull request as ready for review September 13, 2022 10:45
@ematipico ematipico requested a review from a team September 13, 2022 10:45
@ematipico ematipico temporarily deployed to netlify-playground September 13, 2022 10:45 Inactive
@ematipico ematipico temporarily deployed to netlify-playground September 13, 2022 10:51 Inactive
@ematipico ematipico temporarily deployed to netlify-playground September 13, 2022 11:34 Inactive
@ematipico ematipico temporarily deployed to netlify-playground September 13, 2022 11:36 Inactive
Copy link
Contributor

@leops leops left a comment

Choose a reason for hiding this comment

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

Some of the snapshots are detected by Git as having been deleted and added instead of being moved (flipBinExp, inlineVariable, noLabelVar, noNegationElse), I think this may indicate they haven't been moved to the correct directory as the resulting snapshot content is different enough that Git doesn't detect it as the same file

@ematipico ematipico temporarily deployed to netlify-playground September 13, 2022 14:12 Inactive
@ematipico
Copy link
Contributor Author

Some of the snapshots are detected by Git as having been deleted and added instead of being moved (flipBinExp, inlineVariable, noLabelVar, noNegationElse), I think this may indicate they haven't been moved to the correct directory as the resulting snapshot content is different enough that Git doesn't detect it as the same file

I fixed the snapshots, but for noLabelVar there's something strange: the old snapshot didn't have any diagnostic and the new one (the correct one) now has the diagnostic

@ematipico ematipico requested a review from leops September 13, 2022 14:12
@leops
Copy link
Contributor

leops commented Sep 13, 2022

Some of the snapshots are detected by Git as having been deleted and added instead of being moved (flipBinExp, inlineVariable, noLabelVar, noNegationElse), I think this may indicate they haven't been moved to the correct directory as the resulting snapshot content is different enough that Git doesn't detect it as the same file

I fixed the snapshots, but for noLabelVar there's something strange: the old snapshot didn't have any diagnostic and the new one (the correct one) now has the diagnostic

For noLabelVar I think it was the other way around, the file wasn't in the correct directory in the first place, but it is now

Copy link
Contributor

@leops leops left a comment

Choose a reason for hiding this comment

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

The diagnostics for noNegationElse are still missing from the snapshots, I think we should have a safety check in the analyzer tests that verifies the path of the snapshots files correspond to the rule instead of failing silently

@ematipico ematipico force-pushed the refactor/rename-linter-groups branch from a1cc999 to 72937cf Compare September 13, 2022 14:45
@ematipico ematipico temporarily deployed to netlify-playground September 13, 2022 14:45 Inactive
@ematipico ematipico requested a review from leops September 13, 2022 14:50
@ematipico ematipico temporarily deployed to netlify-playground September 13, 2022 14:55 Inactive
@ematipico ematipico merged commit 348f581 into main Sep 13, 2022
@ematipico ematipico deleted the refactor/rename-linter-groups branch September 13, 2022 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载