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

refactor(rome_service): better messaging for rule options #4661

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

ematipico
Copy link
Contributor

Summary

This PR refactors the way we deserialize the options of the lint rules.

I wasn't delighted with the diagnostic messages, there were very unclear and we were showing suggestions that didn't belong to the rule we were scanning.

I created ad-hoc functions to pass down the rule name and use it to emit correct diagnostics. This involves a bit more code, but it gives the user a better message.

Test Plan

Added new test cases and updated the current ones

@netlify
Copy link

netlify bot commented Jul 6, 2023

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 89bf9ee
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64a6bc1999b0880008138697
😎 Deploy Preview https://deploy-preview-4661--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 configuration.

@github-actions github-actions bot added A-Linter Area: linter A-Tooling Area: our own build, development, and release tooling A-Project Area: project configuration and loading labels Jul 6, 2023
@github-actions
Copy link

github-actions bot commented Jul 6, 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 1764 1764 0
Failed 4448 4448 0
Panics 0 0 0
Coverage 28.40% 28.40% 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 ematipico requested review from unvalley and Conaclos July 6, 2023 13:17
@ematipico
Copy link
Contributor Author

ematipico commented Jul 6, 2023

@unvalley I added a new ignore code tag, to signal the code generation to not analyze the code of the snippet.

Comment on lines -25 to -28
- enumMemberCase
- hooks
- maxAllowedComplexity
- strictCase
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Conaclos what do you think? I'd say now the messages are better

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better!

Comment on lines -25 to -28
- enumMemberCase
- hooks
- maxAllowedComplexity
- strictCase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better!

@ematipico ematipico merged commit 4071154 into main Jul 6, 2023
@ematipico ematipico deleted the refactor/rule-options-deserialization branch July 6, 2023 15:46
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