Skip to content
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

Parser: Better names for parser util functions #1547

Merged
merged 1 commit into from Jan 17, 2019

Conversation

IvanGoncharov
Copy link
Member

Based @OlegIlyenko suggestion:
#1545 (comment)

@IvanGoncharov
Copy link
Member Author

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.

@OlegIlyenko I decide to extract it into a separate PR to make review more effective. Can you please review this PR and comment if new names match your suggestion.

@OlegIlyenko
Copy link
Contributor

@IvanGoncharov Thanks a lot for considering my feedback and working this PR!

I think it reads much better now 👍 I like how easy it is to identify which tokens are optional and which are not. I also like how occurrences of skip and expect are unified under the same term.

Just an idea to play around with: I feel that term "expect" (or even more explicit variations expectTerm/expectKeyword) would better communicate the intention. But I'm not sure whether it's just me. So I don't want to emphasize it too much. In a current form it is already a good improvement, I think.

@IvanGoncharov
Copy link
Member Author

Just an idea to play around with: I feel that term "expect" (or even more explicit variations expectTerm/expectKeyword) would better communicate the intention. But I'm not sure whether it's just me. So I don't want to emphasize it too much. In a current form it is already a good improvement, I think.

@OlegIlyenko Not sure I fully understand the proposed naming strategy.
How about we discuss it in term of concrete functions:

  • expect - if match token kind then advance and return token else throw
  • skip - if match token kind then advance else throw
  • skipOptional - if match token kind then advance and return true else return false
  • skipKeyword - if match keyword then advance else throw
  • skipOptionalKeyword - if match keyword then advance and return true else return false

Can you please copy this list and add/delete funtions or rename them if needed?

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented Oct 6, 2018

I mean something like this:

  • expectToken - if match token kind then advance and return token else throw
  • expectOptionalToken - if match token kind then advance and return token else return null/undefined
  • expectKeyword - if match keyword then advance and return token else throw
  • expectOptionalKeyword - if match keyword then advance and return token else return null/undefined

So the idea is to boil it down to only expect* functions which can be used in all scenarios. If return value of these functions is not needed, then it can be safely ignored.

@mjmahone
Copy link
Contributor

mjmahone commented Nov 4, 2018

So I think there's still a little confusion about the proposed terms, especially around "skip" which I usually read as closer to "I think this might be here but it means nothing if it isn't". The original naming scheme was still confusing because of this, I think.

I really like @OlegIlyenko's naming scheme on this, though.

@IvanGoncharov
Copy link
Member Author

I updated PR according @OlegIlyenko last comment.

@IvanGoncharov IvanGoncharov merged commit b3d1f7b into graphql:master Jan 17, 2019
@IvanGoncharov IvanGoncharov added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Jan 17, 2019
@IvanGoncharov IvanGoncharov deleted the lexerUtilFnRename branch February 12, 2019 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants