refactor(rome_analyze): align the syntax of lint suppression with the corresponding diagnostic category #3788
Conversation
✅ Deploy Preview for docs-rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
crates/rome_analyze/src/lib.rs
Outdated
| let signal = DiagnosticSignal::new(move || { | ||
| SuppressionDiagnostic::new( | ||
| file_id, | ||
| category!("suppressions/deprecatedSyntax"), | ||
| range, | ||
| "Suppression is using a deprecated syntax", | ||
| ) | ||
| }); |
There was a problem hiding this comment.
Here this diagnostics could have the tag DiagnosticTags::DEPRECATED_CODE. It doesn't make much sense to keep two different comment styles and should stick with one.
There was a problem hiding this comment.
This is something I considered doing but I ended up not going with it as it made the editor highlight the suppression comment with the deprecated strike-through styling, this made it seem like the comment itself was deprecated which I thought could be confusing to some users.
| } | ||
| } | ||
| DiagnosticKind::Raw(error) => { | ||
| DiagnosticKind::Raw(error.with_tags(DiagnosticTags::FIXABLE)) |
There was a problem hiding this comment.
Are the final tags joined together with this function? Because diagnostics emitted for old suppression comments should deprecated and fixable.
There was a problem hiding this comment.
Yes I've implemented the .with_tags combinator to return a new diagnostic that has both the tags of the inner diagnostics in addition to the ones passed in to the function
a24123d to
fc2e2c7
Compare
…the corresponding diagnostic category
fc2e2c7 to
da9e60c
Compare
Warnings should not cause |
Summary
Fixes #3446
This PR changes the syntax of lint suppression comments from
rome-ignore lint(group/rule)torome-ignore lint/group/ruleto 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_analyzeto 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.