-
Notifications
You must be signed in to change notification settings - Fork 14
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
Create StatisticsClient class #16
Conversation
export class StatisticsClient { | ||
public readonly context: DataStoreContext; | ||
|
||
constructor(context: DataStoreContext) { |
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.
Can we use like this: constructor(private readonly context: DataStoreContext) { } ?
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.
fixed
@@ -0,0 +1,31 @@ | |||
import { DataStoreContext } from "./DataStoreContext"; |
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.
Please, move after Licence
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.
moved
593d2f3
to
f33b02c
Compare
*/ | ||
export class StatisticsClient { | ||
constructor(private readonly context: DataStoreContext) { | ||
this.context = context; |
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.
Please, remove this.context = context;
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.
done
return Promise.reject(new Error(`No layerId provided`)); | ||
} | ||
const coverageRequestBuilder = await this.getRequestBuilder(summaryRequest.catalogHrn) | ||
.catch(error => error); |
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.
.catch(error => error); Maybe should be returned Promise.reject from getSummary ?
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.
fixed
}).catch(this.errorHandler); | ||
} | ||
|
||
private async errorHandler(error: any) { |
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.
could you please not use any?
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.
fixed
private _catalogHrn: string | undefined; | ||
private _layerId: string | undefined; | ||
|
||
get catalogHrn(): string | undefined { |
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.
could you please use getCatalogHrn() to align with C++?
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.
done
get layerId(): string | undefined { | ||
return this._layerId; | ||
} | ||
set layerId(layerId: string | undefined) { |
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.
We don't need this setter? we have withLayerId
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.
removed
4505d3c
to
f772d38
Compare
Create StatisticsClient class, add constructor and unit-tests Add getSummary method to fetch summary data from CoverageApi Create SummaryRequest class to add parameters to the getSummary method Resolves: OLPEDGE-939, OLPEDGE-940 Signed-off-by: ashevchu <[email protected]>
f772d38
to
83e5abe
Compare
Create StatisticsClient class, add constructor and unit-tests
Add getSummary method to fetch summary data from CoverageApi
Create SummaryRequest class to add parameters to the getSummary method
Resolves: OLPEDGE-939, OLPEDGE-940
Signed-off-by: ashevchu [email protected]