Skip to content

Show external API calls in data extensions editor#2263

Merged
koesie10 merged 12 commits intomainfrom
koesie10/data-extensions-editor-calls
Apr 5, 2023
Merged

Show external API calls in data extensions editor#2263
koesie10 merged 12 commits intomainfrom
koesie10/data-extensions-editor-calls

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Apr 4, 2023

This updates the view of the data extensions editor to show a table of possible sources/sinks/flow summaries that can be edited. It's not yet possible to save the changes or load the existing file.

This is quite a big PR because it runs the query and shows the results. However, I don't think splitting this up further would really make this clearer. However, the extension and view parts can be reviewed relatively independently since the only communication between them is through two messages.

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 updates the view of the data extensions editor to show a table of
possible sources/sinks/flow summaries that can be edited. It's not yet
possible to save the changes or load the existing file.
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Generally looking good. I have some comments mainly around code readability but since we're aiming to just get a prototype out a lot of it can be left for later on.

Comment thread extensions/ql-vscode/src/data-extensions-editor/interface.ts
Comment thread extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-module.ts Outdated
Comment thread extensions/ql-vscode/src/data-extensions-editor/interface.ts Outdated
Comment thread extensions/ql-vscode/src/view/data-extensions-editor/DataExtensionsEditor.tsx Outdated
Comment thread extensions/ql-vscode/src/view/data-extensions-editor/MethodRow.tsx Outdated
Comment thread extensions/ql-vscode/src/view/data-extensions-editor/MethodRow.tsx Outdated

await this.showProgress({
message: "Finalizing results",
step: 1450,
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.

What are these numbers?

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.

These numbers are based on what makes it look "nice" in the view. They are out of 1500 since that allows some granularity.

@koesie10 koesie10 requested review from a team and charisk April 5, 2023 09:34
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback, looking good!


await this.showProgress({
message: "Decoding results",
step: 1200,
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.

I think it's worth a comment explaining these numbers as they seem quite arbitrary.

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.

I've added a comment on the showProgress method explaining how this got to be a max step of 1500.

@koesie10 koesie10 enabled auto-merge April 5, 2023 13:15
@koesie10 koesie10 merged commit 97af137 into main Apr 5, 2023
@koesie10 koesie10 deleted the koesie10/data-extensions-editor-calls branch April 5, 2023 14:55
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