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

docs(bigquery): remove location parameter from samples #73

Merged
merged 2 commits into from
Oct 25, 2019
Merged

docs(bigquery): remove location parameter from samples #73

merged 2 commits into from
Oct 25, 2019

Conversation

plamut
Copy link

@plamut plamut commented Oct 19, 2019

Fixes #9499.
Supersedes #9500.

Following the comment on the original PR, I am submitting the changes directly to a related PR to avoid merge conflicts when merging all the work into the main Google Cloud Python repository.

@plamut
Copy link
Author

plamut commented Oct 19, 2019

@emar-kar I left the location argument in a few places in the samples where I thought we want to keep it to demonstrate its use cases (e.g. explicitly setting the location of a new dataset).

@plamut plamut mentioned this pull request Oct 19, 2019
Closed
4 tasks
@plamut
docs(bigquery): remove location parameter from samples
915aaa4 
The parameter can sometimes confuse new BigQuery developers. Since
location autodetection now works pretty well, the parameter can be
removed from code samples for better clarity, except where the samples
want to explicitly demonstrate its usage.
@plamut
Copy link
Author

plamut commented Oct 21, 2019 •
edited
Loading

@emar-kar Any chance of merging this? This way we avoid potential merge conflicts when you guys make any changes to the new-samples branch.

Thanks! :)

@emar-kar
Copy link

@plamut I think you need to ask Tim Swast for the review first. He should confirm that the location parameter should be deleted not only at the query methods. And I assume he will ask to merge this PR directly into google-cloud-python repo, instead of double merge through another PR.

@plamut
Copy link
Author

plamut commented Oct 21, 2019 •
edited
Loading

@emar-kar Since my set of changes is more trivial, I'm fine with waiting until the new-samples branch is merged in the main repo. Will be easier to resolve any further merge conflicts on my side.

@tswast
Copy link

tswast commented Oct 25, 2019

I'm fine reviewing this PR here now that I found it in my notifications. :-)

tswast
tswast suggested changes Oct 25, 2019
View reviewed changes
bigquery/samples/client_query_batch.py Outdated Show resolved Hide resolved
bigquery/samples/tests/test_create_job.py Outdated Show resolved Hide resolved
@emar-kar emar-kar merged commit 04555f9 into MaxxleLLC:new-samples Oct 25, 2019
@plamut plamut deleted the iss-9499-b branch October 25, 2019 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tswast tswast tswast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@plamut @emar-kar @tswast