-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add listReport #1053
Add listReport #1053
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
b45b3d4
to
dd758fd
Compare
d424bbd
to
3e77336
Compare
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.
Reviewed 1 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcorilla and @riemanli)
src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportsService.kt
line 344 at r2 (raw file):
} val metrics: List<Metric> =
You don't need this. You already have the map so externalIdToMetricMap.values would work.
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcorilla and @tristanvuong2021)
src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportsService.kt
line 344 at r2 (raw file):
Previously, tristanvuong2021 (Tristan Vuong) wrote…
You don't need this. You already have the map so externalIdToMetricMap.values would work.
This is for the case of listReports
. The externalIdToMetricMap
provided from listReports
is for all metrics used across reports, so I need to get the list of metrics that are just for the specific report.
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.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jcorilla)
3e77336
to
1bfde43
Compare
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.
Reviewed 1 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jcorilla, @riemanli, and @stevenwarejones)
src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportsService.kt
line 718 at r3 (raw file):
val isValidPageSize = source.pageSize != 0 && source.pageSize >= MIN_PAGE_SIZE && source.pageSize <= MAX_PAGE_SIZE
nit this could be simplified: source.pageSize in 1..MAX_PAGE_SIZE
Code quote:
source.pageSize != 0 && source.pageSize >= MIN_PAGE_SIZE && source.pageSize <= MAX_PAGE_SIZE
1bfde43
to
66693c0
Compare
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.
Reviewable status: 2 of 4 files reviewed, all discussions resolved (waiting on @jcorilla, @Marco-Premier, @stevenwarejones, and @tristanvuong2021)
src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportsService.kt
line 718 at r3 (raw file):
Previously, Marco-Premier (marcopremier) wrote…
nit this could be simplified:
source.pageSize in 1..MAX_PAGE_SIZE
Thanks for the suggestion! Done.
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.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jcorilla, @Marco-Premier, and @stevenwarejones)
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.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jcorilla and @stevenwarejones)
66693c0
to
f3b6495
Compare
No description provided.