-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Core Rest] Add pagination helper for rest clients @azure-rest/core-client-paging #15831
Conversation
sdk/agrifood/agrifood-farming-rest/review/agrifood-farming.api.md
Outdated
Show resolved
Hide resolved
*/ | ||
function checkPagingRequest(response: PathUncheckedResponse) { | ||
if (!Http2xxStatusCodes.includes(response.status)) { | ||
throw ( |
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.
Just wondering - is there some centralized logic that already exists in core-client that's used for throwing based on the status of a response? If so - would it make sense to use that here?
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.
That's a good point, I'll be working on an LRO helper next. If I find we could leverage this, I'll add it as a core helper so it can be referenced centrally
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.
I like this approach a lot! Per our offline discussion I think we can go ahead with creating the helper packages.
/// <reference lib="esnext.asynciterable" /> | ||
|
||
import { Client, HttpResponse, PathUncheckedResponse } from "./"; | ||
import "@azure/core-paging"; |
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.
this is to make sure the below type import doesn't get erased and we don't load the async iterator polyfill?
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.
I was following what other libraries did when using core-paging. I think it is just to load the polyfill before we try to import anything from core-paging. I'm removing it since we now support Node 10+ so the polyfill shouldn't be needed anymore
[Symbol.asyncIterator]() { | ||
return this; | ||
}, | ||
byPage: () => { |
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.
what about PageSettings
for controlling page size? Or is that not standard enough at the moment?
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.
Unfortunately autorest's x-ms-pageable
doesn't provide a way to set PagingSettings :(
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
/azp run js - core - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run js - core - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
A few last small nits but looks good!
/azp run prepare-pipelines |
Azure Pipelines successfully started running 1 pipeline(s). |
Paginate response is a helper function to handle pagination for the user. Given a response that contains a body with a link to the next page and an array with the current page of results, this helper returns a PagebleAsyncIterator that can be used to get all the items or page by page.
This helper is intended to be wrapped by generated code to provide more context, such as types and pagination options.
The following code snippet shows the user experience consuming a generated client that re-exposes the pagination function with extra context.
Here's how the generated SDK would consume and export this helper.
In order to provide better typings, the library that consumes
paginateResponse
can wrap it providing additional types. For example a code generator may consume and export in the following wayNote: This work implements the standard pagination strategy supported by Autorest
x-ms-pageable
extension,If a Service uses a non-standard pagination strategy. The SDK author would be able to implement it's own pagination helper in a similar way as we do here for paginateRequest and export a
paginate
function