-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat(gapic-generator): Change REGAPIC pagination algorithm to the standard algorithm for non-compute clients #1143
Conversation
f7ca806
to
c394de3
Compare
c394de3
to
c5b8976
Compare
…ndard algorithm for non-compute clients
c5b8976
to
87d7ed8
Compare
return "::Gapic::Rest::PagedEnumerable<#{pagination.paged_element_doc_type}>" if paged? | ||
if paged? | ||
elem_type = compute_pagination&.paged_element_doc_type | ||
elem_type ||= (lro? ? "::Gapic::Operation" : @main_method.paged_response_type) |
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 will always return false
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.
Are you sure? It's not possible to be both paged?
and lro?
? The logic in MethodPresenter#lro?
seems to account for the case of both.
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.
you're checking for lro?
2 lines up and bailing out if it's true though
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.
Oh 🤦🏻♂️ you're right. Okay I don't understand fully the cases here, but I guess we'll retain the current logic.
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.
LGTM with some comments
See internal issue b/382502247. This updates the pagination detection algorithm for REST/REGAPIC clients so it complies with AIP-158. It does not affect compute, which should remain on the custom algorithm.
This will cause breaking changes in 70-ish GAPICs, where pagination behavior for some REST RPC methods will change. Thus, this should be merged and applied with care.