Skip to content

Update samples to support latest Google Cloud Python #656

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
Merged
merged 13 commits into from
Nov 15, 2016
Merged

Update samples to support latest Google Cloud Python #656

merged 13 commits into from
Nov 15, 2016

Conversation

theacodes
Copy link
Contributor

@theacodes theacodes commented Nov 15, 2016 •
edited
Loading

(I recommend reviewing commit by commit)

Jon Wayne Parrott added 9 commits November 15, 2016 10:17
Update to latest google-cloud-python
6351867 
Change-Id: I11180caa0f97743fc3475e05fc44d5686702aabd
Fix some minor nox issues
d106640 
Change-Id: I78ceda896df08c433620b3a40e21fef7bb72f4a2
Update datastore samples for new google-cloud-python
91847ca 
Change-Id: I07d16616e373d925f6ce5e9e47ab2fb7d073eb59
Update bigquery samples to use latest google-cloud
1374be3 
Change-Id: I6cbcca11be84baa7c40eed14b342cc916b9116d9
Update dns samples to use latest google-cloud
20d9de9 
Change-Id: Ibd8cc3b651e666c5c5e7d9537a690cf4e9acc03a
Update logging samples to use latest google-cloud
11575bc 
Change-Id: I3ea731e4022610802b262a93197dee17b2e09838
Update pubsub samples to use latest google-cloud
f691fbd 
Change-Id: Iac57ac0708fadc11f358c1599f4d1ce94b0dac8b
Update storage samples to use latest google-cloud
22e3d53 
Change-Id: Ic9731e804792aeaeee51ca87ab7dc5971fe7ef5b
Fix lint
03b27e8 
Change-Id: Ibb9430ba18ac4c73608495c6ff3b050582040512
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 15, 2016
jerjou
jerjou approved these changes Nov 15, 2016
View reviewed changes
Copy link
Contributor

@jerjou jerjou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Cool beans - I like that the iterators page for you now :D (wow github just auto-completed my smiley into a dagger... sheesh. calm down, github.)

Just a couple minor suggestions.

datastore/cloud-client/snippets.py

return tasks, cursor
tasks = list(page)
next_cusor = query_iter.next_page_token
Copy link
Contributor

Choose a reason for hiding this comment

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

next_cusor? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

nox.py
requirements = [
requirement for requirement in requirements if not
is_ignored(requirement)]

Copy link
Contributor

Choose a reason for hiding this comment

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

Could put this conditional in the previous comprehension, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather not.

nox.py
"""Ignores certain libraries."""
for library in IGNORED_LIBRARIES:
if requirement.startswith(library):
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

Could do this more succinctly:

return any(requirement.startswith(l) for l in IGNORED_LIBRARIES)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it, but it's almost too clever. I'm going to leave as-is for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't care either way but don't think it's too clever, a bit more English-esque if anything.

Fix lint
3568981 
Change-Id: I059b64397a783643a21ee75bf2d54082ce5cb23c
@waprin
Copy link
Contributor

waprin commented Nov 15, 2016

LGTM, shoutout to @dhermes for a lot of nice refactors that make these samples look a lot nicer.

@theacodes
Copy link
Contributor Author

@dpebot merge when travis passes, please.

@dpebot
Copy link
Collaborator

dpebot commented Nov 15, 2016

Okay! I'll merge when all statuses are green.

@dpebot dpebot added the automerge Merge the pull request once unit tests and other checks pass. label Nov 15, 2016
Vision quickstart no longer fails on Py3
49405df 
Change-Id: Iab5bd07b405cf57fa7407efcfa188d1320e5f867
dhermes
dhermes reviewed Nov 15, 2016
View reviewed changes
bigquery/cloud-client/snippets.py
# Load at most 25 results. You can change the max_results argument to load
# more rows from BigQuery, but note that this can take some time. It's
# preferred to use a query.
rows = table.fetch_data(max_results=25)
Copy link
Contributor

Choose a reason for hiding this comment

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

You may prefer to just consume the iterator right here:

rows = list(table.fetch_data(max_results=25))

There is too much suspense between this and the for row in rows (when the request(s) actually get made)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, done.

@theacodes theacodes removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 15, 2016
Jon Wayne Parrott added 2 commits November 15, 2016 14:02
Address review comment.
ce4c736 
Change-Id: I33997d42c6202c7f916bcebd4c5a9499392ac4db
Fix bigquery installed app sample
ffdf1ca 
Change-Id: I9609daebbd2bfd62706193e80fde245bd1ae2566
@theacodes theacodes added the automerge Merge the pull request once unit tests and other checks pass. label Nov 15, 2016
@theacodes theacodes merged commit cf1b498 into master Nov 15, 2016
@theacodes theacodes deleted the google-cloud-update branch November 15, 2016 22:58
@m-strzelczyk m-strzelczyk mentioned this pull request Nov 18, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhermes dhermes dhermes left review comments

@waprin waprin waprin left review comments

@jerjou jerjou jerjou approved these changes

Assignees

@jerjou jerjou

@waprin waprin

@elibixby elibixby

@ryanmats ryanmats

Labels
automerge Merge the pull request once unit tests and other checks pass. 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.
8 participants
@theacodes @waprin @dpebot @dhermes @jerjou @googlebot @elibixby @ryanmats