Handling for Data Preparation actions #1789
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
| @@ -858,6 +892,123 @@ defaultNotebookRuntimeOptions: | |||
| }); | |||
| }); | |||
|
|
|||
| suite("data_preparations", () => { | |||
There was a problem hiding this comment.
Nit:
| suite("data_preparations", () => { | |
| suite("data preparations", () => { |
core/main_test.ts
Outdated
| @@ -57,6 +57,40 @@ class TestConfigs { | |||
|
|
|||
| 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.
Add logic to handle Data preparation definitions and parse data preparation YAML into the proto representation of the operation.