Correct @flow types. Refactor some Parser code for stricter typing. #896
Conversation
marcianx
changed the title
|
|
||
| // 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.
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.
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.
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.
Good idea separate this from the parseFunction which might return null.
|
@marcianx thanks for keeping the changes small. |
Toward #892.