Paginator template and unit test #3681
Conversation
| class PageIterator | ||
| { | ||
| public: | ||
| PageIterator() = default; |
There was a problem hiding this comment.
why have a default constructor? deafult initialization doesnt sound valid
There was a problem hiding this comment.
Hm, I need it because the default constructor creates the "end" iterator for me to check if both iterator have m_atEnd == true and reached the end
| m_currentOutcome = OperationTraits::Invoke(*m_client, m_request); | ||
| } | ||
|
|
||
| ServiceClient* m_client{}; |
There was a problem hiding this comment.
if its a raw pointer who owns it? who deletes it?
There was a problem hiding this comment.
It would be the caller that owns it
S3Client client; // Caller
auto paginator = client.ListObjectsPaginated(request); // Borrows pointer
for (const auto& outcome : paginator) { ... }
// Paginator destroyed
|
|
||
| bool operator!=(const PageIterator& other) const | ||
| { | ||
| return !(m_atEnd && other.m_atEnd); |
There was a problem hiding this comment.
why for not equals are you only comparing the boolean atEnd and not the other members? specifically m_currentOutcome currently two iterators completely unrelated to eachother would be equal as long as they were not at the end, which is incorrect i think.
There was a problem hiding this comment.
In this comparison we're only detecting "end of iteration". In practice, customers wont be comparing two active iterator, they only compare against the end() iterator.. So we can just check if an iterator is used up.
If we were to compare iterators from different paginators we would have to add overhead and compare m_client pointers.
|
|
||
| bool operator==(const PageIterator& other) const | ||
| { | ||
| return !(*this != other); |
There was a problem hiding this comment.
lets flip this so that equals actually compares and not equals is the negation of equals
There was a problem hiding this comment.
Youre right, let me flip this
| if (!m_currentOutcome.IsSuccess()) | ||
| { | ||
| m_atEnd = true; | ||
| return *this; |
There was a problem hiding this comment.
how will the error in this case be shown to the customer? your test only works because the initial request fails in the constructor, what if the second or subsequent requests fail? lets add a test for this and fix this.
There was a problem hiding this comment.
Gotcha, I added a new test TestHandlesErrorOnSecondPage to test that errors stored in (m_currentOutcome) on subsequent pages (page 2) are properly shown to customers.
customers can check outcome.IsSuccess() for the error
| { | ||
|
|
||
| template <class ServiceClient, class OperationRequest, class OperationTraits> | ||
| class PagePaginator |
There was a problem hiding this comment.
think we want to make this a iterator_trait yeah? i.e.
The template can be specialized for user-defined iterators so that the information about the iterator can be retrieved even if the type does not provide the usual typedefs.
i.e.
class MyIterator {
public:
using iterator_category = std::forward_iterator_tag;
using value_type = int;
using difference_type = std::ptrdiff_t;
using pointer = int*;
using reference = int&;
// Iterator operations...
};
this is so that std lib algorithms and other iterator based libraries can treat it as a iterator officially
There was a problem hiding this comment.
Added new iterator_trait
changed wild pointer to shared pointer
2 participants
Issue #, if available:
Description of changes:
Adds a generic
PagePaginatorutility class for iterating through paginated API responses using C++ range-based for loops.Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.