Skip to content
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(api_core): provide a 'raw_page' field for page_iterator.Page #9486

Merged
merged 8 commits into from
Nov 1, 2019

Conversation

software-dov
Copy link
Contributor

@software-dov software-dov commented Oct 16, 2019

Some paginated response messages include additional fields that users
may wish to inspect. This change stores the raw page when constructing a page_iterator.Page and provides an accessor.

In addition to the new and modified unit tests, hand experimentation generates expected behavior:

from google.cloud import automl
client = automl.AutoMlClient()

result = client.list_datasets(...)
for page in result.pages:
    print(f"{page.raw_page}")

prints individual pages.

Some paginated response messages include additional fields that users
may wish to inspect.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 16, 2019
@software-dov software-dov changed the title Provide a 'raw_page' field for page_iterator.Page feat: provide a 'raw_page' field for page_iterator.Page Oct 16, 2019
@software-dov software-dov changed the title feat: provide a 'raw_page' field for page_iterator.Page feat(api_core): provide a 'raw_page' field for page_iterator.Page Oct 16, 2019
@crwilcox
Copy link
Contributor

Thanks @software-dov, any chance that this would be doable off of Iterator instead of Page so it could be used like:

response = client.list_resources(request)
handle_raw_response(p.raw_response)

@software-dov
Copy link
Contributor Author

My two main concerns in no particular order are

  1. As few differences from the surface exposed by the microgenerator as possible.
    If breaks are inevitable, prioritize breaks that don't require drastic changes in the microgenerator.
  2. Ergonomic user interface

With "ease of implementation" being a non-voting tie-breaker.

Making the raw page (or transparently mapped attributes via getattr) visible from Iterator matches 1 with the minor caveat that 'page_token' isn't always a flattened parameter in either surface. I'm not entirely happy with the resulting interface, however.

With the microgenerator surface, touching internal fields of the page forces you to do your own pagination, something like

while True:
    response = client.paginated_method(request)
    handle_page(response)
    if not response.next_page_token:
        break
    request.page_token = response.next_page_token

which doesn't look great to me. Again, modulo complications with flattening, this is the interface that exposing raw_page in the iterator would be forced to mimic.

I guess I'm not sure I understand the circumstances where the topmost level raw_page is preferable to

response = client.paginated_method(request)
for p in response.pages:
    handle_page(p)
    for elt in p:
        handle_element(elt)

@software-dov
Copy link
Contributor Author

Open review and design bump

api_core/google/api_core/page_iterator.py Outdated Show resolved Hide resolved
api_core/google/api_core/page_iterator.py Outdated Show resolved Hide resolved
api_core/google/api_core/page_iterator.py Outdated Show resolved Hide resolved
api_core/google/api_core/page_iterator.py Outdated Show resolved Hide resolved
api_core/tests/unit/test_page_iterator.py Outdated Show resolved Hide resolved
api_core/tests/unit/test_page_iterator.py Show resolved Hide resolved
api_core/tests/unit/test_page_iterator.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending greep CI (you can ignore the Cloud Build failure).

api_core/.coveragerc Outdated Show resolved Hide resolved
api_core/tests/unit/test_page_iterator.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: core cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants