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

feat(rome_service): diagnostic severity for rules #2941

Merged
merged 14 commits into from
Aug 4, 2022

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Jul 27, 2022

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 the Diagnostic 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:

  • if the rule is recommended, by default it will emit and error;
  • if the rule is not recommended, by default it will emit a warning;

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 fledged Diagnostic, but it doesn't have the severity. When calling into_diagnostic, a severity must be passed and it will emit a Diagnostic. 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 #2948

Pending 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 to Severity::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:

{
  "linter": {
    "enabled": true,
    "rules": {
      "recommended": true,
      "js": {
        "rules": {
          "noDeadCode": "error"
        }
      }
    }
  }
}

notice that noDeadCode is an error.
Screenshot 2022-07-28 at 10 24 55

Instead, given the following configuration:

{
  "linter": {
    "enabled": true,
    "rules": {
      "recommended": true,
      "js": {
        "rules": {
          "noDeadCode": "warn"
        }
      }
    }
  }
}

Notice now how the LSP shows a warning instead.

Screenshot 2022-07-28 at 10 24 34

Updated documentation for the website and the snapshot tests.

@ematipico ematipico temporarily deployed to aws July 27, 2022 16:53 Inactive
@github-actions
Copy link

github-actions bot commented Jul 27, 2022

@github-actions
Copy link

github-actions bot commented Jul 27, 2022

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 12397 12397 0
Failed 3860 3860 0
Panics 0 0 0
Coverage 76.26% 76.26% 0.00%

@cloudflare-workers-and-pages
Copy link

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

Deploying with  Cloudflare Pages  Cloudflare Pages

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

View logs

@ematipico ematipico force-pushed the feature/diagnostics-for-rules branch from b700178 to eeb43da Compare July 28, 2022 07:46
@ematipico ematipico temporarily deployed to aws July 28, 2022 07:46 Inactive
@ematipico ematipico temporarily deployed to aws July 28, 2022 09:23 Inactive
@ematipico ematipico temporarily deployed to aws July 28, 2022 09:31 Inactive
@ematipico ematipico marked this pull request as ready for review July 28, 2022 09:31
@ematipico ematipico requested review from leops and xunilrj as code owners July 28, 2022 09:31
@ematipico ematipico requested a review from a team July 28, 2022 09:31
@ematipico ematipico force-pushed the feature/diagnostics-for-rules branch from 5760869 to 4f5158e Compare July 28, 2022 12:54
@ematipico ematipico temporarily deployed to aws July 28, 2022 12:55 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.

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

@ematipico ematipico force-pushed the feature/diagnostics-for-rules branch from 4f5158e to 265dbe5 Compare August 3, 2022 11:48
@ematipico ematipico temporarily deployed to aws August 3, 2022 11:48 Inactive
@ematipico ematipico added A-Linter Area: linter A-Project Area: project configuration and loading labels Aug 3, 2022
@ematipico ematipico force-pushed the feature/diagnostics-for-rules branch from 4cc3c59 to 9caee78 Compare August 3, 2022 14:09
@ematipico ematipico temporarily deployed to aws August 3, 2022 14:09 Inactive
@ematipico ematipico temporarily deployed to aws August 3, 2022 14:14 Inactive
@ematipico ematipico temporarily deployed to aws August 3, 2022 15:14 Inactive
@ematipico ematipico force-pushed the feature/diagnostics-for-rules branch from 434d6f4 to 62305d9 Compare August 4, 2022 08:02
@ematipico ematipico requested a review from leops August 4, 2022 08:02
@ematipico ematipico temporarily deployed to aws August 4, 2022 08:02 Inactive
@ematipico ematipico force-pushed the feature/diagnostics-for-rules branch from a218285 to f477bd5 Compare August 4, 2022 11:59
@ematipico ematipico requested a review from leops August 4, 2022 11:59
@ematipico ematipico temporarily deployed to aws August 4, 2022 11:59 Inactive
@ematipico ematipico merged commit 584c468 into main Aug 4, 2022
@ematipico ematipico deleted the feature/diagnostics-for-rules branch August 4, 2022 12:21
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.
Labels
A-Linter Area: linter A-Project Area: project configuration and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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