-
Notifications
You must be signed in to change notification settings - Fork 161
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
fix: union with parsedModules #2311
base: main
Are you sure you want to change the base?
Conversation
@@ -63,7 +65,11 @@ export function openContext( | |||
parser: Parser, | |||
parsedModules?: A.AstModule[], | |||
): CompilerContext { | |||
const modules = parsedModules ?? parseModules(sources, parser); | |||
const parsedSources = parseModules(sources, parser); |
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 would rather make parsedModules
required, and pass parseModules(sources, parser)
in regular build.
// } | ||
// } | ||
|
||
function makeModule(): A.AstModule { |
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.
It seems like this should be a set of definitions in src/ast/no-src-factory.ts
or even a reuse of existing code for this.
It will be extremely tedious to update manually in case AST definitions change. This code should eventually be generated from AST types.
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.
Agreed. I will migrate all the makeX
functions into that new factory, because this will be a common problem in all PRs involving testing: each one of them defines their own makeX
functions.
Is there an example on how to generate code for the makeX
functions just from the AST types? Do you mean like a pre-compilation step, similar to yarn gen
so that all makeX
functions are generated?
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.
Actually, there was a AstUtil
factory. I think I'll place the makeX
functions there.
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.
^
Issue
Closes #2310.
Checklist