Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Removing Iterator and Page subclasses.#2558

Merged
dhermes merged 2 commits into
googleapis:mastergoogleapis/google-cloud-python:masterfrom
dhermes:no-more-iterator-subclassesdhermes/google-cloud-python:no-more-iterator-subclassesCopy head branch name to clipboard
Oct 18, 2016
Merged

Removing Iterator and Page subclasses.#2558
dhermes merged 2 commits into
googleapis:mastergoogleapis/google-cloud-python:masterfrom
dhermes:no-more-iterator-subclassesdhermes/google-cloud-python:no-more-iterator-subclassesCopy head branch name to clipboard

Conversation

@dhermes

@dhermes dhermes commented Oct 18, 2016

Copy link
Copy Markdown
Contributor

Instead require Iterator takes:

  • a well-formed path for the request
  • a callable to convert a JSON item to native obj.
  • (optional) the key in a response holding all items
  • (optional) a page_start (acts as proxy for Page.__init__)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 18, 2016
@dhermes dhermes mentioned this pull request Oct 18, 2016
Closed
@theacodes

theacodes commented Oct 18, 2016

Copy link
Copy Markdown
Contributor

@dhermes the general idea here LGTM.

@dhermes

dhermes commented Oct 18, 2016

Copy link
Copy Markdown
Contributor Author

@tseaver PTAL. I'd really like to get the list_foo() migration done

@dhermes
Removing Iterator and Page subclasses.
8cc9341 
Instead require `Iterator` takes:
- a well-formed path for the request
- a callable to convert a JSON item to native obj.
- (optional) the key in a response holding all items
- (optional) a `page_start` (acts as proxy for `Page.__init__`)
@dhermes dhermes force-pushed the no-more-iterator-subclasses branch from db650b7 to 8cc9341 Compare October 18, 2016 16:39
This was referenced Oct 18, 2016
Merged
Merged
@daspecster

daspecster commented Oct 18, 2016

Copy link
Copy Markdown
Contributor

Ditto @jonparrott.

tseaver
tseaver reviewed Oct 18, 2016
View reviewed changes

@tseaver tseaver left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Except for the odd bit of having item_to_value be optional, this looks right to me.

Comment thread core/google/cloud/iterator.py
:param item_to_value: (Optional) Callable to convert an item from JSON
into the native object. Assumed signature
takes an :class:`Iterator` and a dictionary
holding a single item.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread core/google/cloud/iterator.py Outdated

:type items_key: str
:param items_key: The key used to grab retrieved items from an API
response. Defaults to :data:`DEFAULT_ITEMS_KEY`.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes

dhermes commented Oct 18, 2016

Copy link
Copy Markdown
Contributor Author

@tseaver Anything else?

@dhermes

dhermes commented Oct 18, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@tseaver So I'll add "(Optional)" to the docstring and make item_to_value a positional / required argument. (UPDATE: And fix the lint error. Wrong PR.)

Good to merge after those changes are in?

@tseaver

tseaver commented Oct 18, 2016

Copy link
Copy Markdown
Contributor

@dhermes

Good to merge after those changes are in?

Yup.

@dhermes
Making item_to_value a required argument.
b6f5328 
Also adding "(Optional)" to items_key docstring.
@dhermes

dhermes commented Oct 18, 2016

Copy link
Copy Markdown
Contributor Author

OK, waited an hour for Travis. Merging. (There is a lot to do after.)

@dhermes dhermes merged commit e575f81 into googleapis:master Oct 18, 2016
@dhermes dhermes deleted the no-more-iterator-subclasses branch October 18, 2016 20:21
@dhermes dhermes mentioned this pull request Nov 14, 2016
Merged
richkadel pushed a commit to richkadel/google-cloud-python that referenced this pull request May 6, 2017
@dhermes
Merge pull request googleapis#2558 from dhermes/no-more-iterator-subc…
cb5bb26 
…lasses

Removing Iterator and Page subclasses.
parthea pushed a commit that referenced this pull request Jun 4, 2023
@dhermes
Merge pull request #2558 from dhermes/no-more-iterator-subclasses
a001cc1 
Removing Iterator and Page subclasses.
parthea pushed a commit that referenced this pull request Oct 21, 2023
@crwilcox @busunkim96
fix: Use different versions of pytest for python 2 and python3 [(#2558)…
f86f985 
…](GoogleCloudPlatform/python-docs-samples#2558)

* fix: Use different versions of pytest for python 2 and python3

* fix: delete extra pytest dep

* fix: update pytest dependencies in requirements.txt
parthea pushed a commit that referenced this pull request Nov 24, 2025
@dhermes
Merge pull request #2558 from dhermes/no-more-iterator-subclasses
86f60f7 
Removing Iterator and Page subclasses.
parthea pushed a commit that referenced this pull request Mar 9, 2026
@dhermes
Merge pull request #2558 from dhermes/no-more-iterator-subclasses
21686ca 
Removing Iterator and Page subclasses.
ohmayr added a commit that referenced this pull request Mar 13, 2026
@ohmayr
chore: fix service yaml command in nox sessions (#2558)
2e334b3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@tseaver tseaver tseaver left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@tseaver tseaver

@theacodes theacodes

Labels

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 @daspecster @tseaver @googlebot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.