8000 feat(rome_js_analyze): noNonoctalDecimalEscape by unvalley · Pull Request #4618 · rome/tools · GitHub
[go: up one dir, main page]
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
unvalley merged 19 commits into
rome:mainfrom
unvalley:no-nonoctal-decimal-escape
Jun 30, 2023
Merged

feat(rome_js_analyze): noNonoctalDecimalEscape#4618
unvalley merged 19 commits into
rome:mainfrom
unvalley:no-nonoctal-decimal-escape

Conversation

@unvalley
@unvalley unvalley commented Jun 25, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

Summary

Closes #3989

Implement noNonoctalDecimalEscape

Test Plan

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

@netlify
netlify Bot commented Jun 25, 2023 •
edited
Loading
Copy link
Copy Markdown

✅ 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
@ematipico ematipico left a comment
Copy link
Copy Markdown
Contributor

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 :)

Comment thread crates/rome_js_analyze/Cargo.toml Outdated
serde = { version = "1.0.136", features = ["derive"] }
serde_json = { version = "1.0.74" }
smallvec = { workspace = true }
regex = "1.8.4"
@ematipico ematipico Jun 26, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

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.

@unvalley unvalley Jun 26, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

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.

@unvalley unvalley Jun 29, 2023
Copy link
Copy Markdown
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

Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Conaclos
Conaclos suggested changes Jun 26, 2023
View reviewed changes
Comment thread CHANGELOG.md Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
@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
unvalley commented Jun 29, 2023
Copy link
Copy Markdown
Contributor Author

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

@unvalley
unvalley requested review from Conaclos and ematipico June 29, 2023 01:45
ematipico
ematipico approved these changes Jun 29, 2023
View reviewed changes
@ematipico ematipico left a comment
Copy link
Copy Markdown
Contributor

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 :)

Comment thread Cargo.lock
@ematipico ematipico Jun 29, 2023
Copy link
Copy Markdown
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.

Comment thread crates/rome_js_analyze/src/analyzers/nursery/no_nonoctal_decimal_escape.rs Resolved
@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
@Conaclos Conaclos left a comment
Copy link
Copy Markdown
Contributor

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

2 more reviewers
@ematipico ematipico ematipico approved these changes
@Conaclos Conaclos Conaclos approved these changes
Reviewers whose approvals may not affect merge requirements

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
0