Paginate ListPipeline gRPC [CORE-2133] #9825
Conversation
acohen4
force-pushed
the
acohen4/paginate-pipelines
branch
from
739ab57
to
425c0a3
Compare
acohen4
changed the title
acohen4
force-pushed
the
acohen4/paginate-pipelines
branch
from
c3e5637
to
779c4c4
Compare
|
|
||
| // Page indicates which page of a certain size to return. If page is left empty, | ||
| // all of the selected pipelines will be returned. | ||
| PipelinePage page = 7; |
There was a problem hiding this comment.
If it’s not too late, I think it might be a good idea to reconsider some of the names here. Is this a pipeline page, or is this a definition or request or specification for a pipeline page?
There was a problem hiding this comment.
I actually like the name page here. It's clear and short. What would you call it?
There was a problem hiding this comment.
Maybe PageSpec? Not a huge deal, I just found an earlier version of the code a little surprising to read. But this right now seems okay:
offset = request.Page.PageIndex * request.Page.PageSize
acohen4
force-pushed
the
acohen4/paginate-pipelines
branch
from
c855679
to
3ec7da9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9825 +/- ##
==========================================
- Coverage 58.93% 58.92% -0.01%
==========================================
Files 586 586
Lines 70673 70683 +10
==========================================
- Hits 41653 41652 -1
- Misses 28487 28499 +12
+ Partials 533 532 -1 ☔ View full report in Codecov by Sentry. |
src/server/pps/server/api_server.go
Outdated
| @@ -3012,12 +3012,21 @@ func (a *apiServer) listPipeline(ctx context.Context, request *pps.ListPipelineR | |||
| }) | |||
| }) | |||
| } | |||
| var offset int64 | |||
| if request.Page != nil { | |||
| offset = request.Page.PageIndex * request.Page.PageSize | |||
There was a problem hiding this comment.
You can reduce this whole var … if … { … } to offset := request.GetPage().GetPageIndex() * request.GetPage().GetPageSize(). It is mildly ugly to call request.GetPage twice, but it’s not the end of the world, and you could always assign it to a variable if you like, and then use that variable down at line 3,027.
No description provided.