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

refactor(rome_analyze): deserialize options early #4541

Merged
merged 7 commits into from
Jun 10, 2023

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented May 29, 2023

Summary

This PR refactors how the options of the linter are parsed/deserialized and passed to the actual linter.

Before, the serialization was done only when the rule was called, which caused various problems:

  • the parsing and deserialisation of the options of the rules were done inside the rule;
  • we had to pass a partial stringified JSON of the configuration. When this is parsed, the diagnostics were all wrong because we don't have the original configuration anymore;
  • we couldn't create a decent JSON schema of the options of the configuration;

With this PR, we now serialize all the options early when we parse and deserialize the configuration. The options are passed to the analyzer as a Box<dyn Any>, and then we do a downcast when needed. This is safe to do because the AnalyzerOptions are mandatory to pass, and the 'static restriction is always met.

In order to achieve this result, I had to add more restrictions to Rule::Options, such as Default and Clone.

Plus, I refactored the shape of the options of useExhaustiveDependencies and useHookAtTopLevel. The previous version didn't work out very well with Bpaf, and it was difficult to understand the numbers. With this new version, we can also document the value of the fields when we emit the JSON schema of the configuration.

Considering that these two rules are still under nursery, I'd say we don't need to create a migration for that, although I will create a changelog line with a hint of how to move to the new version of the configuration.

Test Plan

  • added some new test cases for the new configuration;
  • changed how we pass and test the configuration in the testing suite of rome_js_analyzer. Now we pass a full-fledged rome.json file, so we can use utility functions that we use in rome_service;
  • the tests of the current rules should still pass;

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented May 29, 2023

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit a41466b
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6475fbc2d150680007886289
😎 Deploy Preview https://deploy-preview-4541--docs-rometools.netlify.app/playground
📱 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.

@github-actions github-actions bot added A-Linter Area: linter A-Project Area: project configuration and loading A-Tooling Area: our own build, development, and release tooling labels May 29, 2023
@ematipico ematipico force-pushed the refactor/rule-options branch from a8d7a36 to 1fd6b5f Compare May 29, 2023 16:33
@github-actions
Copy link

github-actions bot commented May 29, 2023

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47810 47810 0
Failed 1053 1053 0
Panics 0 0 0
Coverage 97.84% 97.84% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6212 6212 0
Passed 1763 1763 0
Failed 4449 4449 0
Panics 0 0 0
Coverage 28.38% 28.38% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 573 573 0
Failed 66 66 0
Panics 0 0 0
Coverage 89.67% 89.67% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13121 13121 0
Failed 4103 4103 0
Panics 0 0 0
Coverage 76.18% 76.18% 0.00%

@ematipico
Copy link
Contributor Author

ematipico commented May 29, 2023

Parser conformance results on ubuntu-latest%0A%0A### js/262%0A%0A| Test result | main count | This PR count | Difference |%0A| :---------: | :----------: | :-----------: | :--------: |%0A| Total | 48863 | 48863 | 0 |%0A| Passed | 47796 | 47796 | 0 |%0A| Failed | 1067 | 1067 | 0 |%0A| Panics | 0 | 0 | 0 |%0A| Coverage | 97.82%25 | 97.82%25 | 0.00%25 |%0A%0A### jsx/babel%0A%0A| Test result | main count | This PR count | Difference |%0A| :---------: | :----------: | :-----------: | :--------: |%0A| Total | 40 | 40 | 0 |%0A| Passed | 37 | 37 | 0 |%0A| Failed | 3 | 3 | 0 |%0A| Panics | 0 | 0 | 0 |%0A| Coverage | 92.50%25 | 92.50%25 | 0.00%25 |%0A%0A### symbols/microsoft%0A%0A| Test result | main count | This PR count | Difference |%0A| :---------: | :----------: | :-----------: | :--------: |%0A| Total | 6212 | 6212 | 0 |%0A| Passed | 1763 | 1763 | 0 |%0A| Failed | 4449 | 4449 | 0 |%0A| Panics | 0 | 0 | 0 |%0A| Coverage | 28.38%25 | 28.38%25 | 0.00%25 |%0A%0A### ts/babel%0A%0A| Test result | main count | This PR count | Difference |%0A| :---------: | :----------: | :-----------: | :--------: |%0A| Total | 639 | 639 | 0 |%0A| Passed | 573 | 573 | 0 |%0A| Failed | 66 | 66 | 0 |%0A| Panics | 0 | 0 | 0 |%0A| Coverage | 89.67%25 | 89.67%25 | 0.00%25 |%0A%0A### ts/microsoft%0A%0A| Test result | main count | This PR count | Difference |%0A| :---------: | :----------: | :-----------: | :--------: |%0A| Total | 17224 | 17224 | 0 |%0A| Passed | 13127 | 13127 | 0 |%0A| Failed | 4097 | 4097 | 0 |%0A| Panics | 0 | 0 | 0 |%0A| Coverage | 76.21%25 | 76.21%25 | 0.00%25 |

@nikeee you recently changed the CI output, I believe we forgot to add newlines to the output. Would you be available to do a follow-up PR to fix it?

@nikeee
Copy link
Contributor

nikeee commented May 29, 2023

@ematipico I'll have a look

Edit: #4542

@ematipico ematipico merged commit 7b65492 into main Jun 10, 2023
@ematipico ematipico deleted the refactor/rule-options branch June 10, 2023 08:57
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 A-Tooling Area: our own build, development, and release tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants