tools icon indicating copy to clipboard operation
tools copied to clipboard

refactor(rome_analyze): align the syntax of lint suppression with the corresponding diagnostic category

Open leops opened this issue 3 years ago • 2 comments

Summary

Fixes #3446

This PR changes the syntax of lint suppression comments from rome-ignore lint(group/rule) to rome-ignore lint/group/rule to align with how lint categories are presented in diagnostics. I've tried to keep backwards compatibility by continuing to parse the previous syntax, and emitting a diagnostic with a safe fix when such a comment is encountered by the linter.

Test Plan

I've extended the suppression comment tests in rome_js_analyze to check that both the new and old syntax remain supported, and that a diagnostic is correctly emitted for the deprecated syntax. Additionally I've also used a local build of the language server to apply the automatic fix to all the files with suppression comments that we have.

leops avatar Nov 18 '22 11:11 leops

Deploy Preview for docs-rometools ready!

Name Link
Latest commit da9e60c2ad0a5704c1fcc1abf93b3674bbbf2fd6
Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/637b815484f9fb00087ac96c
Deploy Preview https://deploy-preview-3788--docs-rometools.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Nov 18 '22 11:11 netlify[bot]

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45879 45879 0
Passed 44936 44936 0
Failed 943 943 0
Panics 0 0 0
Coverage 97.94% 97.94% 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 1757 1757 0
Failed 4189 4189 0
Panics 0 0 0
Coverage 29.55% 29.55% 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 12397 12397 0
Failed 3860 3860 0
Panics 0 0 0
Coverage 76.26% 76.26% 0.00%

github-actions[bot] avatar Nov 18 '22 11:11 github-actions[bot]

Also, can these warnings block rome ci? If yes, we should demote them to simple info diagnostics. Otherwise, I think we can merge it!

Warnings should not cause rome ci to fail, only error-level diagnostics will make the CLI return with a non-zero error code

leops avatar Nov 21 '22 13:11 leops