Add basic actions.yaml and notebook support #1595
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Ekrekr
requested review from
tatianaschmidt and
GJMcGowan
and removed request for
BenBirt and
tatianaschmidt
| import { dataform } from "df/protos/ts"; | ||
|
|
||
| /** | ||
| * @hidden |
There was a problem hiding this comment.
This? https://typedoc.org/tags/hidden
So your intention is to hide the class from docs until you're ready, is that right?
There was a problem hiding this comment.
We don't expose these action classes anyways, because users don't access them directly
Line 381 in a4a0a60
core/main.ts
Outdated
| function loadActionConfigs(session: Session, filePaths: string[]) { | ||
| filePaths | ||
| .filter(path => path.startsWith(`definitions${utils.pathSeperator}`)) | ||
| .filter(path => path.endsWith("actions.yaml")) |
There was a problem hiding this comment.
Why two filters instead of path.startsWith && path.endsWith?
| this.proto.config = config; | ||
| } | ||
|
|
||
| public setNotebookContents(contents: string) { |
There was a problem hiding this comment.
this doesn't follow the pattern in other action classes, which do not have setX(...), instead just x(...). i don't have a strong opinion on which style we should use (lewis probably does), but i think we should at least have a consistent API, because this is a public API.
| if (path.endsWith(".ipynb")) { | ||
| const notebookAsJson = JSON.parse(code); | ||
| // TODO(ekrekr): strip notebook cell outputs. | ||
| // TODO(ekrekr): base64 encode the notebook as a string instead. |
| function loadActionConfigs(session: Session, filePaths: string[]) { | ||
| filePaths | ||
| .filter( | ||
| path => path.startsWith(`definitions${utils.pathSeperator}`) && path.endsWith("actions.yaml") |
There was a problem hiding this comment.
this isn't really the spec that was agreed upon - actions.yaml should have a path separator before it
| function loadActionConfigs(session: Session, filePaths: string[]) { | ||
| filePaths | ||
| .filter( | ||
| path => path.startsWith(`definitions${utils.pathSeperator}`) && path.endsWith("actions.yaml") |
There was a problem hiding this comment.
please fix seperator to separator?
| if (!actionConfig.target?.name) { | ||
| if (!actionConfig.target) { | ||
| actionConfig.target = {}; | ||
| } | ||
| actionConfig.target.name = fileNameAsTargetName; | ||
| } |
There was a problem hiding this comment.
IMO this would be clearer as:
if (!actionConfig.target) {
actionConfig.target = {};
}
if (!actionConfig.target.name) {
actionConfig.target.name = fileNameAsTargetName;
}
| }); | ||
| } | ||
|
|
||
| function extractActionDetailsFromFileName( |
There was a problem hiding this comment.
this feels very related to a lot of stuff in utils.ts, and should probably be there
| ): { fileExtension: string; fileNameAsTargetName: string } { | ||
| // TODO(ekrekr): make filename in actions.yaml files not need the `definitions` prefix. In | ||
| // addition, actions.yaml in nested directories should prefix file imports with their path. | ||
| const fileName = path.split("/").slice(-1)[0]; |
There was a problem hiding this comment.
bug: should be using separator
| @@ -193,6 +195,14 @@ export class Session { | |||
| } | |||
| } | |||
|
|
|||
| public notebookAction(notebookConfig: dataform.ActionConfig, notebookContents: string): Notebook { | |||
There was a problem hiding this comment.
this is part of our public API, and should follow that consistently, i.e. notebook()
| if (action.config) { | ||
| action.config.target = target(session.canonicalConfig, name, overrideSchema, overrideDatabase); | ||
| return; | ||
| } |
There was a problem hiding this comment.
i'm confused. why do we need this / what's it for / should it really be here, as opposed to the callsite?
* Add basic actions.yaml and notebook support * Add test for notebook * Add action details extraction from filename, facilitate requiring notebooks, make tests work * Tidy * Fix lint errors * fix * George comments
No description provided.