Skip to content

Use Token::EOF instead of Option<Token> #195

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 10, 2020
Merged

Use Token::EOF instead of Option<Token> #195

merged 3 commits into from
Jun 10, 2020

Conversation

nickolay
Copy link
Contributor

This simplifies codes slightly, removing the need deal with the EOF case explicitly.

@Dandandan what do you think?

@nickolay
Use Token::EOF instead of Option<Token>
5da4198 
The clone kludge in `_ => self.expected("date/time field",
Token::Word(w.clone())))` will become unnecessary once we stop using
a separate match for the keywords, as suggested in
https://github.com/andygrove/sqlparser-rs/pull/193#issuecomment-641607194
@nickolay nickolay mentioned this pull request Jun 10, 2020
Merged
@coveralls
Copy link

coveralls commented Jun 10, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 130885271

  • 106 of 119 (89.08%) changed or added relevant lines in 2 files are covered.
  • 105 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.2%) to 91.943%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 105 118 88.98%
Files with Coverage Reduction New Missed Lines %
src/ast/operator.rs 3 88.46%
src/ast/value.rs 5 87.72%
src/ast/data_type.rs 7 71.43%
src/ast/ddl.rs 8 87.14%
src/ast/query.rs 17 89.82%
src/ast/mod.rs 65 77.84%
Totals Coverage Status
Change from base Build 130660351: 0.2%
Covered Lines: 4051
Relevant Lines: 4406
💛 - Coveralls

@Dandandan
Copy link
Contributor

I like two thing about this change:

  • Makes the encoding of EOF more explicit, rather than relying on Option
  • Reduces the amount of nesting in matches / special casing needed EOF vs normal token

Dandandan
Dandandan reviewed Jun 10, 2020
View reviewed changes
Dandandan
Dandandan reviewed Jun 10, 2020
View reviewed changes
src/parser.rs
self.peek_nth_token(0)
}

/// Return nth non-whitespace token that has not yet been processed
pub fn peek_nth_token(&self, mut n: usize) -> Option<Token> {
pub fn peek_nth_token(&self, mut n: usize) -> Token {
let mut index = self.index;
loop {
index += 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This index += 1 I think would be better at the and of the loop so it can be just self.tokens.get(index), or convert to a normal for loop?
(just looking at the code surrounding the changes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a continue later on, so moving it below won't work. If you have suggestions for improvements, feel free to raise separate PRs.

@nickolay nickolay merged commit 0fe3a8e into apache:master Jun 10, 2020
@nickolay nickolay deleted the pr/token-eof branch June 10, 2020 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dandandan Dandandan Dandandan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nickolay @coveralls @Dandandan