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

fix(rome_js_analyze): noNonoctalDecimalEscape returns single diagnostic #4663

Merged
merged 3 commits into from
Jul 7, 2023
Merged

fix(rome_js_analyze): noNonoctalDecimalEscape returns single diagnostic #4663

merged 3 commits into from
Jul 7, 2023

Conversation

unvalley
Copy link
Contributor

@unvalley unvalley commented Jul 6, 2023

Summary

Addresses https://discord.com/channels/678763474494423051/978209399120216076/1126496823587115018

This PR modifies the noNonoctalDecimalEscape rule and narrows the scope of its fixes. This change eliminates the unusual circumstance of multiple diagnostics being performed on behalf of the user and also rectifies issues with the cargo lintdoc command.

If I understand correctly, the current analyzer should only be able to return one action for each diagnostic.
In this rule, below example \0\8 should have multiple diagnoses and suggest multiple fixes.

const x = "\0\8";
// `\0\8` should be reported and fixed to `\u00008` 
// `\8` should be reported and fixed to `\0\u0038`
// `\8` should be reported and fixed to `\0\\8` (this fix will be removed by this PR because of diagnostic duplication and EscapeBackslash is uncommon change)
// `\8` should be reported and fixed to `\08` (this fix will be removed by this PR because of diagnostic duplication)

Ideally, this rule would allow a unique TextRange to propose a single diagnostic and multiple actions (fixes) for it.
However, this rule in its current state has diagnosed the same place many times.

I remove the code regarding addition of EscapeBackslash for reducing multiple diagnoses anyway. EscapeBackslash is valid change. But fixing the mistake to simple number (8 or 9) Refator is more strait-forward.

Test Plan

cargo test -p rome_js_analyze -- no_nonoctal_decimal_escape

Thought

RuleAdvice can have SuggestionList and CodeSuggestionList. But this isn't used by Rule directly and they need MarkupBuf instead of properties that JsRuleAction requires.

@netlify
Copy link

netlify bot commented Jul 6, 2023

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 90188d0
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64a7075d3cf5dc00089237db
😎 Deploy Preview https://deploy-preview-4663--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.

@unvalley unvalley changed the title fix(rome_js_analyze): noNonoctalDecimalEscape returns single diagnost… fix(rome_js_analyze): noNonoctalDecimalEscape returns single diagnostic Jul 6, 2023
@github-actions github-actions bot added the A-Linter Area: linter label Jul 6, 2023
Comment on lines +153 to +154
let mut seen = HashSet::new();
signals.retain(|rule_state| seen.insert(rule_state.diagnostics_text_range));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here it removes duplicates from the signals by diaagnositcs_text_range used for diagnostics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be more efficient way

@unvalley unvalley marked this pull request as ready for review July 7, 2023 02:06
@unvalley unvalley requested a review from ematipico July 7, 2023 02:09
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Wow, that was a swift fix! Thank you!

@ematipico ematipico merged commit 54b2ab7 into rome:main Jul 7, 2023
@unvalley unvalley deleted the no-nonoctal-decimal-escape-fixes-only-one branch July 7, 2023 08:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants