Skip to content

Load existing data extension YAML in editor#2265

Merged
koesie10 merged 7 commits intomainfrom
koesie10/data-extension-editor-yaml-load
Apr 6, 2023
Merged

Load existing data extension YAML in editor#2265
koesie10 merged 7 commits intomainfrom
koesie10/data-extension-editor-yaml-load

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Apr 4, 2023

This loads in the existing data extension YAML file for the selected database. It only supports the filename we save it to, and will not load it from any other data extension YAML files.

Based on #2264

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

This loads in the existing data extension YAML file for the selected
database. It only supports the filename we save it to, and will not load
it from any other data extension YAML files.
void extLogger.log(`Saved data extension YAML to ${modelFilename}`);
}

protected async readExistingYaml(): Promise<void> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is readExistingModeledMethods() a better name for this? We refer to YAML in function and message names, but that feels like an implementation detail.

Also, usually with readXXX functions I expect something to be retuned. How about loadXXX instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry my comment was covering too many things - what do you think about updating messages as well to drop "yaml" and use "modeledMethods" e.g. setExistingYamlData -> setExistingModeledMethods?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call! I've now moved all methods dealing with YAML from the webview to the extension. This also makes it easier to show an error message, so I've also added a simple error message when the data in the file is invalid.

Comment thread extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts Outdated
Comment thread extensions/ql-vscode/src/view/data-extensions-editor/DataExtensionsEditor.tsx Outdated
Comment thread extensions/ql-vscode/src/data-extensions-editor/yaml.ts
Base automatically changed from koesie10/data-extension-editor-yaml to main April 6, 2023 08:35
@koesie10 koesie10 requested a review from charisk April 6, 2023 08:41
@koesie10 koesie10 enabled auto-merge April 6, 2023 08:59
@koesie10 koesie10 merged commit 20e9370 into main Apr 6, 2023
@koesie10 koesie10 deleted the koesie10/data-extension-editor-yaml-load branch April 6, 2023 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants