Implement pipeMode Parameter to EvalCommand #346
Conversation
…ing TBD @rannn505 - [x] Add `pipeMode?: EvalCommandPipeMode` to `EvalCommandParameters` - [x] Add `pipeMode` to `EvalCommandReturn[KEY]['context']` so that the next `EvalCommand` could identify which to remove - [x] Modify `EvalCommand.run()` to add the new context.pipeMode to its configs. *currently on the ones from the schema. Q.1 answer may change it to override piped configs context.pipeMode* - [x] Create Test Units
| @@ -224,6 +227,14 @@ export class EvalCommand extends Command<EvalCommandReturn> { | |||
| return resultWithTemplates; | |||
| } | |||
|
|
|||
| private removeForwardsFromPipe(result: EvalCommandReturn): EvalCommandReturn { | |||
There was a problem hiding this comment.
The naming of the method is a bit misleading in my opinion because we perform the removal from the result and not the pipe itself. How about one of these?
removeForwardsFromPrevious-> aligns withevalPreviousremoveForwards
There was a problem hiding this comment.
- if you take a look at the method's logic,
pipe[current.context.key]validate we're only removing from the piped configs - removeForwardsFromPrevious -> aligns with evalPrevious i don't understand what you mean here
- sure.
removeForwardsit is
There was a problem hiding this comment.
- Was true until you showed me the forward chain bug. I'll revise. Thanks
There was a problem hiding this comment.
Like in the other PR, let's recycle the EvalCommand usages into a function to keep this file leaner
There was a problem hiding this comment.
what other PR?
please elaborate and be specific here. PR's are not related.
There was a problem hiding this comment.
Please adjust the schema examples to be more generic, perhaps we should always do tests on start.cfgu.json.
| @@ -277,6 +288,7 @@ export class EvalCommand extends Command<EvalCommandReturn> { | |||
| schema: schema.name, | |||
| key, | |||
| cfgu, | |||
| pipeMode, | |||
There was a problem hiding this comment.
I think this is a mistake that leads to a bug. Consider a scenario where the 2nd eval also forwards the same keys as the previous, this case will lead to all the keys being lost due to the logic inside of the removal method.
There was a problem hiding this comment.
Good one 💯
Thanks. I'l revise and test
| const evaluatedConfigs = await new ExportCommand({ pipe: evalQuote }).run(); | ||
| expect(evaluatedConfigs).toStrictEqual({ MY_NAME: 'What?', QUOTE: 'Hi! My name is What?, my name is Who?' }); | ||
| }); | ||
| test("run EvalCommand(pipeMode='forward') and pipe to another, only keys from the second should exists in export", async () => { |
There was a problem hiding this comment.
Please add a test or adjust an existing test to check for the scenario where a 2nd eval overrides the same key from a forwarded eval and ensure that the key is still there.
Co-authored-by: Richard Akman <richard.akman1@gmail.com>
Co-authored-by: Richard Akman <richard.akman1@gmail.com>
…alCommand' into 336-Add-pipeMode-Parameter-to-EvalCommand
Co-authored-by: Richard Akman <richard.akman1@gmail.com>
…alCommand' into 336-Add-pipeMode-Parameter-to-EvalCommand
- Tests
| if (!pipe) { | ||
| return result; | ||
| } | ||
| console.log(result); |
| @@ -308,6 +321,8 @@ export class EvalCommand extends Command<EvalCommandReturn> { | |||
| ...this.evalTemplates(result), | |||
| }; | |||
|
|
|||
| result = this.removeForwards(result); | |||
There was a problem hiding this comment.
thats should be after validation as all Configs must be validated
|
@configu/dev we decided to reproductize this feature, closed until next time. |
QA Report I
Closes #336