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

Removing Iterator and Page subclasses. #2558

Merged
merged 2 commits into from
Oct 18, 2016
Merged

Removing Iterator and Page subclasses. #2558

merged 2 commits into from
Oct 18, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Oct 18, 2016

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
Copy link
Contributor

@dhermes the general idea here LGTM.

@dhermes
Copy link
Contributor Author

dhermes commented Oct 18, 2016

@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
Copy link
Contributor

Ditto @jonparrott.

tseaver
tseaver reviewed Oct 18, 2016
View reviewed changes
Copy link
Contributor

@tseaver tseaver left a comment

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.

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

core/google/cloud/iterator.py

: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
Copy link
Contributor Author

dhermes commented Oct 18, 2016

@tseaver Anything else?

@dhermes
Copy link
Contributor Author

dhermes commented Oct 18, 2016 •
edited
Loading

@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
Copy link
Contributor

tseaver commented Oct 18, 2016

@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
Copy link
Contributor Author

dhermes commented Oct 18, 2016

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tseaver tseaver tseaver left review comments

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