-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Correct @flow types. Refactor some Parser code for stricter typing. #896
Conversation
|
||
// TODO: The real type of `args` is `(?ParseNode)[]`, but doing that breaks | ||
// compilation since some of these arguments are passed to `ordargument` | ||
// below which requires a non-nullable argument. |
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 like this idea. It should be pretty easy to do too.
* @return {ParseNode} | ||
*/ | ||
parseGivenFunction(baseGroup) { | ||
if (baseGroup.isFunction) { |
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 feel like by storing isFunction
on ParseFuncOrArgument
it leads to unnecessary wrapping and unwrapping. What if we create a function that given a ParseNode
would tell us if it was a function? With that, we should be able to get rid of ParseFuncOrArgument
and just use ParseNode
everywhere. This is maybe something to consider for a future PR.
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 intend to do more changes within Parser.js
itself in subsequent PRs while the ParseNode
refactor waits on other PRs to complete. ParseFuncOrArgument
will be replaced with a union of 3 structs per our discussion in #892. We can see whether it makes sense to have a function for an is-function check or simply use foo.type === "fn"
(or "$"
or "arg"
) or some such.
* @param {ParseFuncOrArgument} baseGroup | ||
* @return {ParseNode} | ||
*/ | ||
parseGivenFunction(baseGroup) { |
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.
Good idea separate this from the parseFunction
which might return null
.
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
@marcianx thanks for keeping the changes small. |
Toward #892.