-
-
Notifications
You must be signed in to change notification settings - Fork 471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(linter): implement noUselessStringRaw #4263
Conversation
CodSpeed Performance ReportMerging #4263 will improve performances by 8.53%Comparing Summary
Benchmarks breakdown
|
ffed5b4
to
b768e4a
Compare
|
||
let object = tag.object().ok()?; | ||
let object_expr = object.as_js_identifier_expression()?; | ||
let object_name = object_expr.name().ok()?.text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling text()
on a node allocates a string.
We could avoid it by retrieving the token and the calling text_trimmed_text()
let object_name = object_expr.name().ok()?.text(); | |
let object_name = object_expr.name().ok()?.names().ok()?.value_token().ok()?; | |
let object_name = object_name.text_trimmed_text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated~ maybe you mean text_trimmed()
here.
let object_name = object_expr.name().ok()?.text(); | ||
|
||
let member = tag.member().ok()?; | ||
let member_name = member.as_js_name()?.text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let member_name = member.as_js_name()?.text(); | |
let member_name = member.as_js_name()?.value_token().ok()?; | |
let member_name = member_name.text_trimmed_text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated~
let mut chars = input.chars().peekable(); | ||
while let Some(ch) = chars.next() { | ||
if ch == '\\' { | ||
if let Some('n' | 't' | 'r' | '\\' | '"' | '\'' | '0') = chars.peek() { | ||
return true; | ||
} | ||
} | ||
} | ||
false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterating over chars()
is expensive because this decodes UTF-8 characters.
Here we could directly operate over bytes.
Also, peekable()
has some overhead that can be often avoided.
Here is my suggestion:
let mut chars = input.chars().peekable(); | |
while let Some(ch) = chars.next() { | |
if ch == '\\' { | |
if let Some('n' | 't' | 'r' | '\\' | '"' | '\'' | '0') = chars.peek() { | |
return true; | |
} | |
} | |
} | |
false | |
let mut bytes = input.bytes(); | |
while let Some(c) = bytes.next() { | |
if c == b'\\' { | |
if matches!(bytes.next(), Some(b'n' | b't' | b'r' | b'\\' | b'"' | b'\'' | b'0')) { | |
return true; | |
} | |
} | |
} | |
false |
Actually I wonder even if we could just check for \
because it will be directly interpreted in String.raw
and regular strings. For instance in a regular string \a
is a useless escape, however it will be results in the string a
. While in a raw string this results in the string \a
.
Thus, we could just check if the string contains \
. What do you think?
input.contains('\\')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i agree you, It is logically more reasonable to only check \
b768e4a
to
3dd6827
Compare
There was a problem hiding this 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! Thanks! Could you add a changelog entry?
@@ -51,6 +51,7 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b | |||
- Add [noHeadImportInDocument](https://biomejs.dev/linter/rules/no-head-import-in-document/). Contributed by @kaioduarte | |||
- Add [noImgElement](https://biomejs.dev/linter/rules/no-img-element/). Contributed by @kaioduarte | |||
- Add [guardForIn](https://biomejs.dev/linter/rules/guard-for-in/). Contributed by @fireairforce | |||
- Add [noUselessStringRaw](https://github.com/biomejs/biome/pull/4263). Contributed by @fireairforce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems i have added the changelog here, what else i need to add ? @Conaclos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed it. All is perfect :)
Summary
closes: #3945
Test Plan