Conversation
…ration YAML into the proto representation of the operation.
| config.filename = resolveActionsConfigFilename(config.filename, configPath); | ||
| const dataPreparationContents = nativeRequire(config.filename).asJson; | ||
| const dataPreparationDefinition = parseDataPreparationDefinitionJson(dataPreparationContents); | ||
| this.proto.dataPreparation = dataPreparationDefinition; |
There was a problem hiding this comment.
Because of protobufjs weirdness, it's better to do:
| this.proto.dataPreparation = dataPreparationDefinition; | |
| this.proto.dataPreparation = dataform.DataPreparation.create(dataPreparationDefinition); |
There was a problem hiding this comment.
The parseDataPreparationDefinitionJson method already returns this dataform.DataPreparation entity.
core/actions/data_preparation.ts
Outdated
| // If true, adds the inline assertions of dependencies as direct dependencies for this action. | ||
| public dependOnDependencyAssertions: boolean = false; |
There was a problem hiding this comment.
Supporting this complicates things, and requires a lot more testing area - I'd recommended just not supporting this for now.
There was a problem hiding this comment.
Sounds good, I'll remove this for the time being.
core/main_test.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| suite("data_preparations", () => { |
There was a problem hiding this comment.
Nit:
| suite("data_preparations", () => { | |
| suite("data preparations", () => { |
core/main_test.ts
Outdated
|
|
||
| const EMPTY_NOTEBOOK_CONTENTS = '{ "cells": [] }'; | ||
|
|
||
| const DATA_PREPARATION_CONTENTS = ` |
There was a problem hiding this comment.
Nit: move this under the suite, or because there's only one test, put it in there.
core/session.ts
Outdated
|
|
||
| public dataPreparation(name: string): DataPreparation { | ||
| const dataPreparation = new DataPreparation(); | ||
| dataPreparation.session = this; | ||
| utils.setNameAndTarget(this, dataPreparation.proto, name); | ||
| dataPreparation.proto.fileName = utils.getCallerFile(this.rootDir); | ||
| this.actions.push(dataPreparation); | ||
| return dataPreparation; | ||
| } | ||
|
|
There was a problem hiding this comment.
I would remove method for support in the JS API - this new method isn't covered under any tests as it is
| message DataPreparation { | ||
| // Data preparatiohs can have more than 1 output | ||
| // Used ONLY as an identifiers. All outputs should be listed in the targets | ||
| // section. | ||
| Target target = 8; | ||
| Target canonical_target = 9; | ||
|
|
||
| // Data preparations can have more than 1 output |
There was a problem hiding this comment.
These should be put in the configs.proto file too, or they won't be user overrideable
There was a problem hiding this comment.
Given that the data preparation is self-containing, I don't think this is something that we should allow the user to override.
2 participants
Add logic to handle Data preparation definitions and parse data preparation YAML into the proto representation of the operation.