feat: Address pagination API gap with aligned approach#345
Conversation
sichanyoo
left a comment
sichanyoo
left a comment
There was a problem hiding this comment.
Comments for additional context
| @@ -0,0 +1,40 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
This is the custom enumerator with limited API surface to safeguard against unsafe Enumerable methods
| @@ -4,6 +4,10 @@ module Smithy | |||
| module Schema | |||
| # A module mixed into Structs that provides utility methods for Structure shapes. | |||
| module Structure | |||
There was a problem hiding this comment.
Add each undef to prevent silent data loss in the case pageable_response plugin doesn't run for whatever reason (e.g., customer customization on plugin list that leaves it out)
There was a problem hiding this comment.
Probably better if you turn the above into a code comment for future us.
| @@ -36,6 +36,13 @@ def initialize(response) | |||
| # This yields one response object per API call made. The SDK retrieves additional | |||
| # pages of data to complete the request. | |||
| # | |||
There was a problem hiding this comment.
Updates the PageableResponse module that gets added to Response to return the custom enumerator when not give a block, and if given a block, enumerate using the custom enumerator and yield results. As it has been, PageableResponse#each will paginate over pages instead of items due to uniqueness of AWS background discussed offline.
jterapin
left a comment
jterapin
left a comment
There was a problem hiding this comment.
The overall approach looks good but curious what we can do about the enumerator being created at every method call. 🤔
Also - I was wondering if the PR description could contain overall info about the motivation of this approach and what decisions we make.
We should also have wangrch take a look when he comes back.
| def enum | ||
| Enumerator.new(&@block) | ||
| end |
There was a problem hiding this comment.
The current PageEnumerator creates a new Enumerator on every method call (map, select, first, etc.) via the private enum method. Is there a way to prevent this or was this intentional?
| def reduce(*, &) = enum.reduce(*, &) | ||
|
|
||
| def first(val = (no_arg = true | ||
| nil)) |
There was a problem hiding this comment.
Can we do: def first(*args) if we are just forwarding args?
| @@ -107,16 +126,29 @@ def each_page(&) | |||
| end | |||
There was a problem hiding this comment.
What about returning self here to enable chaining?
| @@ -4,6 +4,10 @@ module Smithy | |||
| module Schema | |||
| # A module mixed into Structs that provides utility methods for Structure shapes. | |||
| module Structure | |||
There was a problem hiding this comment.
Probably better if you turn the above into a code comment for future us.
| require_relative 'smithy-client/paginators/page_enumerator' | ||
| require_relative 'smithy-client/paginators/pageable_response' |
There was a problem hiding this comment.
Ruby convention: a directory paginators/ implies a module Paginators. But PageEnumerator and PageableResponse don't live under a Smithy::Client::Paginators module
Feels longwinded if we add a new module so maybe we should keep this flat.
| response = response.next_page | ||
| @paginator.items(response.data).each(&) | ||
| # @return [PageEnumerator, nil] | ||
| def each_item(&block) |
There was a problem hiding this comment.
If the API does not define items and customer calls .each_item, they'll get a NotImplementedError. Should we document this?
2 participants
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.