Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

First pass tests and impl for PageResult #50

Merged
merged 6 commits into from
May 21, 2019

Conversation

software-dov
Copy link
Contributor

In order to support multiple use cases and extreme customizations,
not to mention making a more modular, well-designed,
and helpful implementation, paginated method results will be based on three classes
and corresponding iterators.

This is the commit for PageResult, which is a gax-owned type that provides a thin, consistent interface
over individual result pages. Its primary purpose is to add C++ iterators over the repeated elements embedded in the page message.

In order to support multiple use cases and extreme customizations,
not to mention making a more modular, well-designed,
and helpful implementation, paginated method results will be based on three classes
and corresponding iterators.

This is the commit for PageResult, which is a gax-owned type that provides a consistent interface
over individual result pages.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 15, 2019
Copy link

@coryan coryan left a comment

Choose a reason for hiding this comment

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

I am not super certain of my comments here, take them with a grain of salt.

gax/pagination.h Show resolved Hide resolved
gax/pagination.h Outdated Show resolved Hide resolved
gax/pagination.h Outdated Show resolved Hide resolved
gax/pagination_test.cc Outdated Show resolved Hide resolved
gax/pagination_test.cc Outdated Show resolved Hide resolved
gax/pagination_test.cc Outdated Show resolved Hide resolved
gax/pagination_test.cc Outdated Show resolved Hide resolved
gax/pagination.h Outdated Show resolved Hide resolved
gax/pagination.h Outdated Show resolved Hide resolved
gax/pagination_test.cc Outdated Show resolved Hide resolved
gax/pagination_test.cc Outdated Show resolved Hide resolved
gax/pagination.h Outdated Show resolved Hide resolved
gax/pagination.h Outdated Show resolved Hide resolved
ElementAccessor is now a template parameter
The type of the iterator over the repeated field is now deduced.
page_result.RawPage().next_page_token());
EXPECT_EQ(page_result.RawPage().operations_size(), 10);

// Move iteration test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore this, fixed in local repo.


iterator(FieldIterator current) : current_(std::move(current)) {}

FieldIterator current_;
Copy link
Contributor

Choose a reason for hiding this comment

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

For my edification, what is the reason for having the using FieldIterator = ... statement be private inside PageResult, but exposed publicly here? Is this intentional, and provides some benefit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not intentional, but it is also not intended to be a user-visible type. It makes more sense when the constructor is private.

Proper access control hygiene is something I'm working on, and I plan on doing a comprehensive pass of the codebase as part of maintenance later on.


FieldIterator current_;
};
// Note: Since the constructor will eventually be private (with friend
Copy link
Contributor

Choose a reason for hiding this comment

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

When is eventually? In followup PRs adding the other classes supporting pagination?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The immediate followup PR is mostly finished and makes the constructor private with friend access granted to other implementation classes. The current setup is partly a memory aid/habit I want to form to prevent overreliance on FRIEND_TEST.

@software-dov software-dov merged commit 4f0d354 into googleapis:master May 21, 2019
@software-dov software-dov deleted the pagination branch May 21, 2019 19:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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