-
Notifications
You must be signed in to change notification settings - Fork 268
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
WIP: input tracking #5496
base: main
Are you sure you want to change the base?
WIP: input tracking #5496
Conversation
Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
146 passing (206ms) 95 failing Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
core/src/template-string/ast.ts
Outdated
value: v.value, | ||
inputs: { | ||
// key might be something like ["var", "foo", "bar"] | ||
// We also add the keypath to get separate keys for ever |
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.
// We also add the keypath to get separate keys for ever | |
// We also add the keypath to get separate keys for every leaf |
core/src/template-string/inputs.ts
Outdated
} | ||
} | ||
|
||
relevantValues.forEach((r) => { |
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 think we should see if we can add this linter rule to our setup: https://github.com/github/eslint-plugin-github/blob/main/docs/rules/array-foreach.md
The reasons for why are well explained in the link.
core/src/template-string/ast.ts
Outdated
constructor( | ||
loc: Location, | ||
public readonly condition: TemplateExpression, | ||
public ifTrue: TemplateExpression, |
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 think ifTrue
is also optional actually, for example for this expression it would be undefined: ${if ...}{else}Hello world{endif}
(It would be undefined because there are no expressions for the then case between the if and the else)
Would be good to add tests for these cases
core/src/template-string/ast.ts
Outdated
) | ||
} | ||
|
||
return mergeInputs(transformed, left, right) |
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.
No need to merge inputs for concatenating arrays. We likely should skip this step for collections.
}) | ||
} | ||
|
||
return new TemplateLeaf({ |
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.
This wipes the inputs of the inner expression;
We need to actually:
return new TemplateLeaf({ | |
return inner |
if (v instanceof LazyValue) { | ||
return new MergeInputsLazily(this.source, v, unwrappedRelevantValues) | ||
} | ||
return mergeInputs(this.source, v satisfies TemplateLeaf<TemplateLeafValue>, ...unwrappedRelevantValues) |
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.
Possible infinite recursion if unwrappedRelevantValues is a collection that contains lazy values?
…nement Co-authored-by: Tim Beyer <TimBeyer@users.noreply.github.com> Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
Co-authored-by: Tim Beyer <TimBeyer@users.noreply.github.com> Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
} else if (allowInvalid) { | ||
return spec | ||
// we shouldn't throw |
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.
Do we really need allowInvalid? Why does it exist?
@@ -335,8 +336,7 @@ export const projectSchema = createSchema({ | |||
), | |||
defaultEnvironment: joi | |||
.environment() | |||
.allow("") | |||
.default("") | |||
.default((parent) => parent.environments[0].name) |
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.
.default((parent) => parent.environments[0].name) | |
.default((parent) => parent.environments?.[0]?.name || "") |
Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com> Co-authored-by: Tim Beyer <TimBeyer@users.noreply.github.com>
Co-authored-by: Tim Beyer <TimBeyer@users.noreply.github.com> Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com>
defaultPath: defaultVarfilePath, | ||
}) | ||
|
||
const environmentConfigContextVariables = new LayeredContext( |
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.
We need tests for some edge cases here:
e.g.
variables:
my
deep:
structure: foo
with override
variables.other.deep.structure: bar
message = res.message | ||
partial = !!res.partial | ||
} else { | ||
// TODO: improve error message | ||
throw new ConfigurationError({ |
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.
Maybe instead of error, it should just result in a plain object with all the values the context contains.
opts: { | ||
...opts, | ||
// Throw an error if we can't find the value in the last layer, unless allowPartial is set | ||
allowPartial: isLastLayer ? opts.allowPartial : false, |
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.
Unnecessary; If we do not find anything, we return CONTEXT_RESOLVE_KEY_NOT_FOUND
instead of throwing.
// In the future we might | ||
// const varsFromFirstConfig = firstConfig.atPath("var") // can contain lazy values | ||
// const actionConfigWithVars = actionConfig.merge(firstConfig) |
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.
// In the future we might | |
// const varsFromFirstConfig = firstConfig.atPath("var") // can contain lazy values | |
// const actionConfigWithVars = actionConfig.merge(firstConfig) |
abstract visitAll(): TemplateExpressionGenerator | ||
} | ||
|
||
export class OverrideKeyPathLazily extends LazyValue { |
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.
Potentially not needed?
Co-authored-by: Thorarinn Sigurdsson <thorarinnsigurdsson@gmail.com> Co-authored-by: Tim Beyer <TimBeyer@users.noreply.github.com>
context: ConfigContext, | ||
opts: ContextResolveOpts | ||
): Record<string, CollectionOrValue<TemplateValue>> { | ||
// Resolve $merge keys, depth-first, leaves-first |
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.
// Resolve $merge keys, depth-first, leaves-first |
What this PR does / why we need it:
Todo:
Special notes for your reviewer: