-
Notifications
You must be signed in to change notification settings - Fork 524
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
Clean up JSON operator tokenizing code #923
Conversation
38f1864
to
4802385
Compare
@@ -1086,8 +1079,8 @@ impl<'a> Tokenizer<'a> { | |||
} | |||
|
|||
/// Tokenize an identifier or keyword, after the first char is already consumed. | |||
fn tokenize_word(&self, first_chars: String, chars: &mut State) -> String { | |||
let mut s = first_chars; | |||
fn tokenize_word(&self, first_chars: impl Into<String>, chars: &mut State) -> String { |
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.
Uses some generics to avoid having to manually construct String
s in several places.
@@ -934,19 +932,14 @@ impl<'a> Tokenizer<'a> { | |||
match chars.peek() { | |||
Some(' ') => Ok(Some(Token::AtAt)), | |||
Some(tch) if self.dialect.is_identifier_start('@') => { | |||
let mut s = ch.to_string(); |
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.
I think the new construction is both less verbose and clearer about intent
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.
cc @izveigor if you have a moment to review these changes I would appreciate it.
Pull Request Test Coverage Report for Build 5579571678
💛 - Coveralls |
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.
Nice cleanup
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.
LGTM
Thanks @Dandandan and @izveigor |
This has some small cleanups of the changes made in #913 by @izveigor