feat: async enumerable and lazy paging#128
Conversation
Also added paging and a cli flag for count on show (that we can default when we get around to the peer config feature)
…ly IAsyncEnumerable implementations
| throw new ArgumentOutOfRangeException(nameof(pageSize), "Must be between 1 and 100"); | ||
|
|
||
| PageSize = pageSize; | ||
| EndCursor = endCursor; |
There was a problem hiding this comment.
I'd have to look at docs again but I think end-cursor is the field that the cursor value comes to us in, in this case it's the cursor we want to start at. I can't imagine we'd do a query from a cursor other than the end-cursor of the previous page but there might be a more context-appropriate name.
There was a problem hiding this comment.
A few options come to mind (in vaguely the order of my preference):
afternextPagepagepageCursornextPageCursorafterCursornextCursorstartCursorThey all have a bit of a stink to them, though endCursor is my least favorite by a small margin since it's named after the source of the value rather than for what the field is.
I like after the most because that's the name of the query field it populates. Next to that nextPage seems like the best description of what the value means.
I don't really like startCursor though I'm not sure why. It is the cursor that specifies where to start so it makes sense; maybe I just don't like that the response we got it from and the response we're expecting will (or at least could) have startCursor values and neither one will match the value of the property.
The rest all seem like okay things to call the thing we know we're talking about, but I don't think they'd be as self-explanatory as good names can be.
| { | ||
| if (value.ReviewThreads.PageInfo.HasNextPage) | ||
| { | ||
| prsWithMoreThreads.Add(value); |
There was a problem hiding this comment.
Now that we’re yielding the other ones out I think prs is the same collection as prs with more threads. We can yield out the completed ones in each iteration of the paging loop and ditch the second collection.
There was a problem hiding this comment.
Actual we can merge the loop behaviours too.
There was a problem hiding this comment.
Still wanna chat about this but wanna get it in in a different PR?
| return new GraphQLHttpRequest(PRSearch.Search.GenerateInvolves(searchParams)); | ||
| if (response.Errors != null) | ||
| { | ||
| Console.WriteLine($"ERROR: {string.Join('\n', response.Errors.Select(x => x.Message))}"); |
There was a problem hiding this comment.
We don't have one registered atm. Was going to but want to chat with you about what we went with. Ideally something minimal
2 participants
Also fixed a bug in details if no checks were present.
Also added a few small sort keys on properties that already exist