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

feat(rome_js_analyze): noNonoctalDecimalEscape #4618

Merged
merged 19 commits into from
Jun 30, 2023
Merged

feat(rome_js_analyze): noNonoctalDecimalEscape #4618

merged 19 commits into from
Jun 30, 2023

Conversation

unvalley
Copy link
Contributor

@unvalley unvalley commented Jun 25, 2023 •
edited
Loading

Summary

Closes #3989

Implement noNonoctalDecimalEscape

Test Plan

  • cargo test -p rome_js_analyze -- no_nonoctal_decimal_escape
    • used ESLint test cases
  • just r

@netlify
Copy link

netlify bot commented Jun 25, 2023 •
edited
Loading

✅ Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 5d4de90
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/649ea54b5babac0008d20bb0

@github-actions github-actions bot added A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Project Area: project configuration and loading labels Jun 25, 2023
@unvalley unvalley marked this pull request as ready for review June 26, 2023 00:59
@unvalley unvalley requested a review from ematipico June 26, 2023 02:23
@ematipico ematipico requested a review from Conaclos June 26, 2023 07:08
ematipico
ematipico reviewed Jun 26, 2023
View reviewed changes
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.

Amazing work @unvalley ! I left few suggestions to polish the code :)

crates/rome_js_analyze/Cargo.toml Outdated
@@ -29,6 +29,7 @@ schemars = { workspace = true, optional = true }
serde = { version = "1.0.136", features = ["derive"] }
serde_json = { version = "1.0.74" }
smallvec = { workspace = true }
regex = "1.8.4"
Copy link
Contributor

@ematipico ematipico Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

regex is a big crate (size and compile time), and we have tried to avoid it for a long time 😅

Now, I am not saying that we shouldn't. How difficult is implementing this rule without regex? I think it's difficult; then I believe it's time to include it.

Would you mind adding regex to the root Cargo.toml? We will need this crate in other places.

Copy link
Contributor Author

@unvalley unvalley Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

regex is a big crate (size and compile time), and we have tried to avoid it for a long time 😅

I got it and I agree with that.
Firstly, let me try to replace it without regex.
If that seems to take quite a while, I'll change the install location of the crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced regex to lexer implementation.
https://github.com/rome/tools/pull/4618/files#diff-eba73c88dc3c490129b5341b6c656e0b2c9831ffe305f19a0c189c79fe0e682aR265

Conaclos
Conaclos suggested changes Jun 26, 2023
View reviewed changes
@github-actions github-actions bot added the A-Core Area: core label Jun 28, 2023
unvalley added 14 commits June 29, 2023 04:19
@unvalley
feat(rome_js_analyze): noNonoctalDecimalEscape
a91d7cb 
refactor

test: parse_escape_sequences

chore: codegen and ready

chore: CHANGELOG.md
@unvalley
fix: use JsStringLiteralExpression as query target
433e9b2
@unvalley
refactor: remove waste passing by reference
a8fd398
@unvalley
refactor: get_unicode_escape receives char as argument
9e0a128
@unvalley
refactor: parse_escape_sequences reads regex directly
669fe7b
@unvalley
docs: update lint rule description
f135d76
@unvalley
chore: comment to describe decimal escape sequence in the string literal
7936351
@unvalley
refactor: safe_replace_by_range returns option
3be6b95
@unvalley
refactor: use node.range instead of text_range
252c837
@unvalley
fix: show diagnostic message more details
0e3e835
@unvalley
refactor: impl try_from to TextRange
e389cd2
@unvalley
refactor: replace regex to lexer
81c84c6
@unvalley
chore: codegen and documentation
b34cfee
@unvalley
fix: waste format to CHANGELOG.md
b7e91df
@unvalley
Copy link
Contributor Author

@ematipico @Conaclos Thank you for the review. I addressed reviewed points. Please take a look.

@unvalley unvalley requested review from ematipico and Conaclos June 29, 2023 01:45
ematipico
ematipico approved these changes Jun 29, 2023
View reviewed changes
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.

Looks good to me. Left few suggestions, feel free to merge it whenever you want :)

Cargo.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I belive these changes should be discarded. Not sure why they are here.

@unvalley @ematipico
Update crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decim…
23aab25 
…al_escape.rs

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Conaclos
Conaclos approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

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

looks good :)

@unvalley unvalley merged commit e4747ee into rome:main Jun 30, 2023
@unvalley unvalley deleted the no-nonoctal-decimal-escape branch June 30, 2023 13:52
@unvalley unvalley mentioned this pull request Nov 27, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ematipico ematipico ematipico approved these changes

@Conaclos Conaclos Conaclos approved these changes

Assignees
No one assigned
Labels
A-Core Area: core A-Diagnostic Area: errors and diagnostics A-Linter Area: linter A-Project Area: project configuration and loading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

noNonoctalDecimalEscape, no-nonoctal-decimal-escape
3 participants
@unvalley @ematipico @Conaclos
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载