-
Notifications
You must be signed in to change notification settings - Fork 1
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
Move code sample for create dataset to samples directory #5
Conversation
# TODO(developer): Set dataset_id to the ID of the dataset to create | ||
# dataset_id = "your-project.your_dataset" | ||
|
||
dataset = bigquery.Dataset(dataset_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please demonstrate setting a location, as is done in the existing snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Location has been added.
# dataset_id = "your-project.your_dataset" | ||
|
||
dataset = bigquery.Dataset(dataset_id) | ||
dataset = client.create_dataset(dataset) # API request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include the additional comment about the Conflict exception from the existing snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the original Conflict exception comment.
bigquery/samples/tests/conftest.py
Outdated
random_dataset_id = "example_dataset_{}_{}".format( | ||
now.strftime("%Y%m%d%H%M%S"), uuid.uuid4().hex[:8] | ||
) | ||
return "{}.{}".format(client.project, random_dataset_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll probably notice that a dataset is created but not deleted when you run these tests. Our tests should clean up after themselves. In this case, we should yield the ID, and then afterwards delete the dataset.
Set both delete_contents=True
and not_found_ok=True
when you call delete_dataset
so that this fixture can be used in samples that create tables within the dataset or don't create a dataset at all (such as if there was a test failure).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed return
to yield
and added the cleanup. Also added the delete_contents=True
and not_found_ok=True
as directed.
In addition, applied not_found_ok=True
to the dataset_id fixture for consistency. Please let me know if it should be removed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you. That will come in handy for the "delete dataset" snippets.
bigquery/samples/create_dataset.py
Outdated
dataset = bigquery.Dataset(dataset_id) | ||
|
||
# TODO(developer): Specify the geographic location where the dataset should reside. | ||
# dataset.location = "US" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be a comment. Specifying an actual location is better, since we can do it. Comments are a last resort (such as when we need to generate a random ID for testing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncommented line 31!
bigquery/samples/tests/conftest.py
Outdated
random_dataset_id = "example_dataset_{}_{}".format( | ||
now.strftime("%Y%m%d%H%M%S"), uuid.uuid4().hex[:8] | ||
) | ||
return "{}.{}".format(client.project, random_dataset_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you. That will come in handy for the "delete dataset" snippets.
bigquery/samples/tests/conftest.py
Outdated
client.delete_dataset(random_dataset, delete_contents=True, not_found_ok=True) | ||
dataset = client.create_dataset(random_dataset_id) | ||
client.delete_dataset(dataset, delete_contents=True, not_found_ok=True) | ||
yield "{}.{}".format(client.project, dataset.dataset_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete needs to come after yield.
Learn more about what this is doing here: https://docs.pytest.org/en/latest/fixture.html#fixture-finalization-executing-teardown-code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you reverted this change, thanks. I do recommend reading that pytest documentation section to understand fixtures better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the link!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I've verified that the docs build with nox -s docs
.
@engelke