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.

Sign up for GitHub

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

Jump to bottom

Moving to iterators in DNS. #2532

Closed
wants to merge 1 commit into from
Closed

Moving to iterators in DNS. #2532

wants to merge 1 commit into from

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Oct 12, 2016 •
edited
Loading

NOTE: Has #2531 as diffbase.

In doing this, I realized it's a bit tedious to have to write two classes (page and iterator) for each method. It may be more user-friendly to collapse back into one class. I think I could do it mostly by just making anything that is accessed via self.page.foo as self.page_foo.

@dhermes dhermes added the api: dns Issues related to the Cloud DNS API. label Oct 12, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 12, 2016
@dhermes
Copy link
Contributor Author

dhermes commented Oct 17, 2016

@daspecster @tseaver @jonparrott PTAL.

There will be about 5 more PRs like this for each package which does page token list_foo().

theacodes
theacodes approved these changes Oct 17, 2016
View reviewed changes
Copy link
Contributor

@theacodes theacodes left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward to me.

@dhermes
Copy link
Contributor Author

dhermes commented Oct 17, 2016

TRAVIS!

@theacodes
Copy link
Contributor

TRAVIS!

No travis for you, I used up all of his cycles in google-auth. :P

@tseaver
Copy link
Contributor

tseaver commented Oct 17, 2016 •
edited
Loading

I don't really care for the inheritance-based iterator approach: would it be better to tackle that first in #2531#2548? Or merge this and then maybe refactor, killing of the subclasses?

@dhermes
Copy link
Contributor Author

dhermes commented Oct 18, 2016

@tseaver Can you weigh in on #2558 ASAP?

@dhermes dhermes mentioned this pull request Oct 18, 2016
Merged
@dhermes dhermes closed this Oct 18, 2016
@dhermes dhermes deleted the dns-iterators branch October 18, 2016 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theacodes theacodes theacodes approved these changes

Assignees

@tseaver tseaver

Labels
api: dns Issues related to the Cloud DNS API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dhermes @theacodes @tseaver @daspecster @googlebot