This repository was archived by the owner on Aug 31, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 653
feat(rome_service): diagnostic severity for rules #2941
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
Deploying with
|
Latest commit: |
a218285
|
Status: | ✅ Deploy successful! |
Preview URL: | https://02f9d2c3.tools-8rn.pages.dev |
Branch Preview URL: | https://feature-diagnostics-for-rule.tools-8rn.pages.dev |
b700178
to
eeb43da
Compare
crates/rome_js_analyze/src/analyzers/js/use_simplified_logic_expression.rs
Outdated
Show resolved
Hide resolved
5760869
to
4f5158e
Compare
leops
reviewed
Jul 28, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the workspace now overrides the severity of diagnostics emitted by the analyzer, should we remove the severity
entirely from the analyzer by returning a partial AnalyzerDiagnostic
object with no severity to be converted into an actual Diagnostic
by the workspace by injecting the severity ? It seems weird to me that rules are even able to set a severity if we're going to overwrite it later on anyway
4f5158e
to
265dbe5
Compare
4cc3c59
to
9caee78
Compare
leops
reviewed
Aug 3, 2022
434d6f4
to
62305d9
Compare
leops
reviewed
Aug 4, 2022
a218285
to
f477bd5
Compare
leops
approved these changes
Aug 4, 2022
IWANABETHATGUY
pushed a commit
to IWANABETHATGUY/tools
that referenced
this pull request
Aug 22, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2946
This PR adds new code generated methods to compute the
Severity
based on the configuration of the rule. This allows to inspect it and decide of theDiagnostic
emitted by the linting phase should be changed.The change of severity is done at
Workspace
level. This should have an added benefit to be language-agnostic and to work on different clients (CLI and LSP).Architectural changes
The severity of the diagnostics is now computed by workspace, this means that when creating a rule, we won't be able to decide there the severity of rule.
The golden rule is the following:
In order to achieve that, I created a new data structure
AnalyzerDiagnostic
(suggested by Leo) which is simply a middle man that holds all the information to create a fully fledgedDiagnostic
, but it doesn't have the severity. When callinginto_diagnostic
, a severity must be passed and it will emit aDiagnostic
.into_diagnostic
is called by the workspace when requesting the diagnostics of a file.In order to correctly build the documentation of the rules, the codegen now creates a slim workspace and via
WorkspaceExt
we are now able to compute the correct severity of the diagnostics.Fixes
This PR also fixes a bug where the recommeded rules were incorrectly computed in some cases.
The rule
useSemplifiedLogicExpression
is not recommended anymore because of #2948Pending works
This change unlocks something really interesting for our adoption. Now that a workspace is needed to compute these diagnostics correctly, the playground is now is not able to correctly compute this information.
In fact, the PR changes also the playground crate. When computing the diagnostic (via
signal.diagnostic()
), we default toSeverity::Error
. This is correct for now because the playground only runs the recommended rules, which will emit errors by design. Once the playground will be able to tweak the configuration of rules, then we will need to create a workspace instance just for the playground.Test Plan
Created new test cases.
I also manually checked the LSP.
Given the following configuration:
notice that

noDeadCode
is an error.Instead, given the following configuration:
Notice now how the LSP shows a warning instead.
Updated documentation for the website and the snapshot tests.