Refactor Runner parameter overloads and resolve review comments#2210
Refactor Runner parameter overloads and resolve review comments#2210rafal-hawrylak wants to merge 1 commit into
Conversation
rafal-hawrylak
requested review from
Ceridan
and removed request for
a team and
Ceridan
rafal-hawrylak
force-pushed
the
pr4b-refactor-runner
branch
from
5811b99 to
d836784
Compare
rafal-hawrylak
force-pushed
the
pr4b-refactor-runner
branch
2 times, most recently
from
5911a46 to
7df9bd9
Compare
rafal-hawrylak
force-pushed
the
pr4b-refactor-runner
branch
from
7df9bd9 to
edb1e7d
Compare
| } | ||
|
|
||
| export interface IExecutionOptions { | ||
| projectDir?: string; |
There was a problem hiding this comment.
Is this projectDir used downstream in latter PRs?
There was a problem hiding this comment.
Looking at this PR I can see that we are adding this field here, and then we are setting it (under some conditions to "."), but I am not sure how it is used. Possibly it is needed latter.
I can guess what this field does in general by the name, but is it needed?
| ).execute(); | ||
| } | ||
|
|
||
| function handleParamsOverloads( |
There was a problem hiding this comment.
This is not obvious to me why we are handling this that way. Can we put some information on what this function is supposed to do?
| optionsOrResult: RunOptionsOrResult, | ||
| partiallyExecutedRunResult: dataform.IRunResult | ||
| ): { options: IExecutionOptions; runResult: dataform.IRunResult } { | ||
| if (optionsOrResult && (optionsOrResult as dataform.IRunResult).actions !== undefined) { |
There was a problem hiding this comment.
If an IRunResult object is passed with actions omitted or undefined, it will inadvertently be misclassified as IExecutionOptions, right? Is that expected?
| private executionTask: Promise<dataform.IRunResult>; | ||
| private readonly executionOptions: IExecutionOptions; | ||
|
|
||
| constructor( |
There was a problem hiding this comment.
Since we want to use create and resume from now on to increase readability, what about making those constructors private (or protected) to enforce usage of those methods? wdyt?
| } | ||
|
|
||
| public cancel() { | ||
| public cancel(reason: CancelReason = "user") { |
There was a problem hiding this comment.
I would suggest preparing explicit map of possible reasons to avoid mistakes, and to make it more readable.
| public cancel(reason: CancelReason = "user") { | ||
| if (!this.skipReason) { | ||
| if (reason === "timeout") { | ||
| this.skipReason = `Run timed out after ${this.graph.runConfig.timeoutMillis / 1000} seconds`; |
There was a problem hiding this comment.
Is it possible that graph.runConfig.timeoutMillis is not defined? I am wondering if this might cause NaN to be displayed.
2 participants
Pre follow up of #2184