-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collect webview messages async #4405
Conversation
stub(dvcReader, 'dag').resolves('') | ||
|
||
const pipeline = buildExperimentsPipeline({ | ||
const { experiments, messageSpy } = await buildExperimentsWebview({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] After reading this test I had no idea why it didn't use the builder so moved it across.
|
||
messageSpy.resetHistory() | ||
|
||
const movedGroup = 'params:params.yaml' | ||
|
||
const tableChangePromise = experimentsUpdatedEvent(experiments) | ||
const messageSent = waitForSpyCall(messageSpy, messageSpy.callCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] I had to add this new util as the data for the message is now gathered async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where possible we still rely on the old util (experimentsUpdatedEvent(experiments)
)
const selectedColumns = this.columns.getSelected() | ||
|
||
const showOnlyChanged = this.columns.getShowOnlyChanged() | ||
private async getWebviewData(): Promise<TableData> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function getWebviewData
has 62 lines of code (exceeds 40 allowed). Consider refactoring.
eadc12e
to
3251d1d
Compare
3251d1d
to
6096ba7
Compare
selectedRevisions, | ||
template: this.getTemplatePlots(selectedRevisions) | ||
}) | ||
const webview = this.getWebview() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[F] although this function is duplicated it is not worth the hassle that a base class would bring
6096ba7
to
52b45dd
Compare
@@ -68,9 +68,13 @@ export class WebviewMessages { | |||
this.update = update | |||
} | |||
|
|||
public sendWebviewMessage() { | |||
public async sendWebviewMessage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Identical blocks of code found in 2 locations. Consider refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Code Climate has analyzed commit d880a15 and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.3% (0.0% change). View more on Code Climate. |
Follow up to #4402 (comment)
This PR makes the collection of experiments and plots webview messages async.