Integrate JiT compilation into CLI runtime#2184
Conversation
rafal-hawrylak
force-pushed
the
pr4b-jit-runtime
branch
2 times, most recently
from
0cc664d to
72e576f
Compare
rafal-hawrylak
force-pushed
the
pr4b-jit-runtime
branch
from
72e576f to
f9c7136
Compare
| } | ||
|
|
||
| export class Runner { | ||
| private static handleParamsOverloads( |
There was a problem hiding this comment.
- Please don't insert static functions the class.
- I don't think having it static vs having it as a standalone function brings any value.
| optionsOrResult: RunOptionsOrResult, | ||
| partiallyExecutedRunResult: dataform.IRunResult | ||
| ): { options: IExecutionOptions; runResult: dataform.IRunResult } { | ||
| if (optionsOrResult && "actions" in optionsOrResult) { |
There was a problem hiding this comment.
Please only use "'key' in X" checks only for types with signature '{[key: string]: ...}`.
This will break on some JS compilers that use member-renaming optimizations.
Use optionsOrResult.actions === undefined
| // Populated when the runner cancels execution (timeout or external cancel). | ||
| // Surfaced as errorMessage on SKIPPED tasks so users can see why an action | ||
| // never ran or stopped mid-flight. | ||
| private skipReason: string = ""; |
There was a problem hiding this comment.
I don't think using raw strings for error handling is a good idea. Please use string literal types
| options: { projectDir: "." } | ||
| }; | ||
| } | ||
| const options = { ...(optionsOrResult as IExecutionOptions) }; |
There was a problem hiding this comment.
Why not just assignment with assertion? Only to populate default?
| private readonly dbadapter: dbadapters.IDbAdapter, | ||
| private readonly graph: dataform.IExecutionGraph, | ||
| private readonly executionOptions: IExecutionOptions = {}, | ||
| optionsOrResult: RunOptionsOrResult = {}, |
There was a problem hiding this comment.
I am not sure what is the motivation of complicating the code with mixed type resolution in class. Probably these two should be separate functions returning Runner instances.
| // shape of tasks the action would have produced. If the action has | ||
| // no built tasks (e.g. JiT-only), still emit one so the cause is | ||
| // visible in the JSON output. | ||
| tasks: (pendingAction.tasks && pendingAction.tasks.length > 0 |
There was a problem hiding this comment.
Please use ternary operators sparingly and only for simple statements. This is unnecessary complex code.
| this.getBigQueryExecutionOptions(action) | ||
| ); | ||
|
|
||
| if (jitResponse.table) { |
There was a problem hiding this comment.
This if else branch sequence should be a function returning tasks.
| } else { | ||
| callback(null, res.response); | ||
|
|
||
| // Guard against double-initialization in some environments (e.g. Bazel) |
There was a problem hiding this comment.
Can we please avoid these hacks and keep initialization idempotent, please?
| ], | ||
| processFn: async argv => { | ||
| const projectDir = argv[projectDirMustExistOption.name]; | ||
| const logger = new Logger(!argv[jsonOutputOption.name]); |
There was a problem hiding this comment.
Please don't mix refactoring with new functionality introduction.
2 participants
Part of #2110 - [Integrate JiT compilation into CLI]