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
| 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?
|
|
||
| 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.
|
|
||
| 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
| file(GLOB AWS_NET_SRC "${CMAKE_CURRENT_SOURCE_DIR}/aws/net/*.cpp") | ||
| file(GLOB HTTP_SRC "${CMAKE_CURRENT_SOURCE_DIR}/http/*.cpp") | ||
| file(GLOB UTILS_SRC "${CMAKE_CURRENT_SOURCE_DIR}/utils/*.cpp") | ||
| file(GLOB UTILS_PAGINATION_SRC "${CMAKE_CURRENT_SOURCE_DIR}/utils/pagination/*.cpp") |
There was a problem hiding this comment.
there is no /utils/pagination/*.cpp" path or files, why is this here?
| 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.
| { | ||
|
|
||
| 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
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.