Skip to content

Parser: Add 'skipKeyword' function #1545

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 1 commit into from
Oct 5, 2018
Merged

Parser: Add 'skipKeyword' function #1545

merged 1 commit into from
Oct 5, 2018

Conversation

IvanGoncharov
Copy link
Member

based on 'expectOptionalKeyword' from #1541

@IvanGoncharov
Parser: Add 'skipKeyword' function
59792e5 
based on 'expectOptionalKeyword' from graphql#1541
@IvanGoncharov IvanGoncharov requested a review from mjmahone October 4, 2018 13:10
@IvanGoncharov
Copy link
Member Author

@OlegIlyenko I had a few review comment about expectOptionalKeyword from #1541 but didn't want increase the scope of this PR so I extracted it out.
Can you please take a look?

IvanGoncharov
IvanGoncharov commented Oct 4, 2018
View reviewed changes
src/language/parser.js
*/
function expectKeyword(lexer: Lexer<*>, value: string): Token {
function skipKeyword(lexer: Lexer<*>, value: string): boolean {
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we already have skip/expect pair with matching functionality I think we need to have skipKeyword/expectKeyword.

mjmahone
mjmahone approved these changes Oct 4, 2018
View reviewed changes
Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

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

This makes sense. Thanks!

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented Oct 4, 2018 •
edited
Loading

I found the terminology around "skip" a bit confusing. "skip" by itself does not imply that the token is optional. So when i saw the usage of skip function it was a bit misleading.

I personally believe that this part of code might benefit from skip + skipOptional + keyword and "expect" alternatives. Purely based on the names, I understood "expect" as something to be used in cases where I'm interested in the token value and "skip" for cases when I just want to jump over the token, but I'm not interested in its value.

Just my 2 cents.

@IvanGoncharov IvanGoncharov merged commit 26c9874 into graphql:master Oct 5, 2018
@IvanGoncharov IvanGoncharov deleted the expectOptionalKeyword branch October 5, 2018 15:38
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Oct 5, 2018
@IvanGoncharov
Parser: Better names for parser util functions
ff59fc1 
Based @OlegIlyenko suggestion:
graphql#1545 (comment)
@IvanGoncharov IvanGoncharov mentioned this pull request Oct 5, 2018
Merged
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jan 17, 2019
@IvanGoncharov
Parser: Better names for parser util functions
c7b2852 
Based @OlegIlyenko suggestion:
graphql#1545 (comment)
IvanGoncharov added a commit that referenced this pull request Jan 17, 2019
@IvanGoncharov
Parser: Better names for parser util functions (#1547)
b3d1f7b 
Based @OlegIlyenko suggestion:
#1545 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjmahone mjmahone mjmahone approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@IvanGoncharov @OlegIlyenko @mjmahone @facebook-github-bot