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.

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

refactor(bigquery): update code samples to use strings for table and dataset IDs #9136

Merged
merged 87 commits into from
Oct 15, 2019

Conversation

emar-kar
Copy link
Contributor

Towards #8989
This PR contains five snippets:

  • client_list_jobs
  • client_query
  • copy_table
  • table_exists
  • table_insert_rows

mf2199 and others added 30 commits August 8, 2019 23:11
*.rst    +
*.py    +
test    +
conf   +
Methods were divided into 3 files:
- add label
- get labels
- delete labels

*.rst - docs updated
tests passed successfully
minor corrections, 'dataset_exists' moved to the 'Getting a Dataset' section
minor corrections, 'dataset_exists' moved to the 'Getting a Dataset' section
grammar fix
grammar fix
minor corrections, removed extra comments
minor corrections, removed extra comments
deleted 'dataset_exists' and 'table_exists' methods
Added additional asserts into the test + refactoring of the main file.
cosmetic chgs by 'black'
Chged an assertion parameter
@plamut
Copy link
Contributor

plamut commented Sep 19, 2019

@emar-kar Of course... I was apparently looking at snippets.py when writing the review comment, my bad. The number of commits is huge, and in retrospect I should have probably looked at the final set of changes right from the start.

@emar-kar
Copy link
Contributor Author

@plamut yeah, the amount of commits is really huge. This is my bad. I’ll fix it with the next part.

@emar-kar emar-kar changed the title BigQuery: Update code samples to use strings for table and dataset IDs refactor(bigquery): Update code samples to use strings for table and dataset IDs Sep 25, 2019
@emar-kar emar-kar changed the title refactor(bigquery): Update code samples to use strings for table and dataset IDs refactor(bigquery): update code samples to use strings for table and dataset IDs Sep 25, 2019
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 25, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 25, 2019
# table_id = "your-project.your_dataset.your_table_name"

orig_table = client.get_table(table_id) # Make an API request.
dataset = client.get_dataset(dataset_id) # Make an API request.
Copy link
Contributor

Choose a reason for hiding this comment

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

Calls to get_table and get_dataset are unnecessary. Let's use

# TODO(developer): Set source_table_id to the ID of the original table.
# source_table_id = "your-project.source_dataset.source_table"

# TODO(developer): Set destination_table_id to the ID of the destination table.
destination_table_id = "your-project.destination_dataset.destination_table"

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 also decided to move the num_rows assertion to the test file.
code_1

print("The query data:")
for row in query_job:
# Row values can be accessed by field name or index
print(row)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's demonstrate that fields can be accessed by name or index.

Suggested change
print(row)
print("name={}, count={}".format(row[0], row["count"]))

# TODO(developer): Construct a BigQuery client object.
# client = bigquery.Client()

query = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we want to show more than one field in the print logic, let's select more than one column.

Suggested change
query = (
query = """
SELECT name, SUM(number) as total_people
FROM `bigquery-public-data.usa_names.usa_1910_2013`
WHERE state = 'TX'
GROUP BY name, state
ORDER BY total_people DESC
LIMIT 20
"""

client_query.client_query(client)
out, err = capsys.readouterr()
assert "The query data:" in out
assert "Row(" in out
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're using the usa_1910_2013 table, the data won't change. We can use a specific value in our tests.

Suggested change
assert "Row(" in out
assert "name=James, count=272793" in out

@emar-kar emar-kar added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 7, 2019
@emar-kar emar-kar added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 8, 2019
@emar-kar emar-kar removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 8, 2019
@emar-kar emar-kar requested a review from tswast October 8, 2019 11:40
@tswast
Copy link
Contributor

tswast commented Oct 8, 2019

Thanks for your patience. I've been travelling a lot lately, but now I'm back.

Re: Make and API request vs Makes an API request, I'd interpret these in two ways:

  1. (imperative) You, the developer, should Make and API request.
  2. (descriptive) This line of code Makes an API request.

Since our code samples are included in how-to guides, our technical writing style guide requires (1) imperative.

@emar-kar
Copy link
Contributor Author

emar-kar commented Oct 8, 2019

Yeah, sorry. I pushed the commit with -s modification today. I'll revert it tomorrow. Thank you, appreciate your help.

@emar-kar
Copy link
Contributor Author

emar-kar commented Oct 9, 2019

@tswast One more thing with the comments. I also added Waits for the job to complete lines in #9212. Maybe it is required to be without -s too?

@tswast
Copy link
Contributor

tswast commented Oct 10, 2019

@tswast One more thing with the comments. I also added Waits for the job to complete lines in #9212. Maybe it is required to be without -s too?

Yes, without the -s would more closely match our tech writing style guide.

@emar-kar emar-kar merged commit 01f6826 into googleapis:master Oct 15, 2019
@emar-kar emar-kar deleted the second-five-v2 branch October 16, 2019 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants