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

deprecate(bigquery): deprecate client.dataset() in favor of DatasetReference #7753

Merged
merged 4 commits into from
Jan 27, 2020
Merged

deprecate(bigquery): deprecate client.dataset() in favor of DatasetReference #7753

merged 4 commits into from
Jan 27, 2020

Conversation

tswast
Copy link
Contributor

@tswast tswast commented Apr 19, 2019 •
edited
Loading

Now that all client methods that take a DatasetReference or
TableReference also take a string, the client.dataset() method is
unnecessary and confusing.

Closes #8989.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 19, 2019
@tswast
Copy link
Contributor Author

tswast commented Apr 19, 2019

@crwilcox I was inspired by our conversation today.

Note: I've created this as a draft pull request because after I made this commit, I realized we still call .dataset() 96 times in our samples at docs/snippets.py, and that's after @lbristol88 already moved a bunch over to the new "just use a string instead of a reference" way of doing things! I would feel better about adding the deprecation notice after we update the rest of our samples.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 26, 2019
@busunkim96
Copy link
Contributor

@tswast Gentle ping to see if there are any updates on this PR.

@tswast
Copy link
Contributor Author

tswast commented Jun 10, 2019

@busunkim96 I've worked with @sduskis to allocate some contractor resources for the necessary code sample updates before we unleash this PR.

Is it an issue if we keep this open for now?

@busunkim96
Copy link
Contributor

@tswast Nope, I just wanted to check. 😸 Thanks!

@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Jul 3, 2019
@tswast tswast mentioned this pull request Aug 7, 2019
Closed
@tswast tswast closed this Dec 27, 2019
@tswast tswast deleted the bq-dot-dataset-deprecation branch December 27, 2019 21:21
@tswast tswast restored the bq-dot-dataset-deprecation branch December 29, 2019 00:04
@tswast tswast reopened this Dec 29, 2019
@tswast tswast force-pushed the bq-dot-dataset-deprecation branch from 074af14 to 5a1bab8 Compare January 23, 2020 20:18
@tswast
deprecate(bigquery): deprecate client.dataset() in favor of Dataset…
cec97ff 
…Reference

Now that all client methods that take a `DatasetReference` or
`TableReference` also take a string, the `client.dataset()` method is
unnecessary and confusing.
@tswast tswast force-pushed the bq-dot-dataset-deprecation branch from 5a1bab8 to cec97ff Compare January 23, 2020 20:19
@tswast tswast changed the title Document client.dataset() deprecation. deprecate(bigquery): deprecate client.dataset() in favor of DatasetReference Jan 23, 2020
@tswast tswast marked this pull request as ready for review January 23, 2020 20:20
@tswast tswast requested review from a team, plamut and shollyman January 23, 2020 20:20
plamut
plamut reviewed Jan 24, 2020
View reviewed changes
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

There are still a lot of client.dataset() calls in snippets.py, but that file is deprecated, IIRC.

The rest looks good, aside from a remark on a possible grammar thingy.

bigquery/google/cloud/bigquery/client.py Outdated
Comment on lines 360 to 363
This method is deprecated. Construct a
:class:`~google.cloud.bigquery.dataset.DatasetReference` using its
constructor or use a string where previously a reference object was
used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use the standard Sphinx directive for deprecations?
The text itself is already clear, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't appear to change formatting all that much:

image

Pushed the change in case it makes things a little more machine-readable or something, though.

bigquery/google/cloud/bigquery/client.py Outdated Show resolved Hide resolved
@plamut
Copy link
Contributor

plamut commented Jan 24, 2020 •
edited
Loading

The test_client_load_partitioned_table snippet test failure could be unrelated, happens on another PR, too.

Might still be worth investigating, though.

Edit: Also happens on master, thus indeed unrelated to this PR.

Update: #10195 fixes it.

@tswast @plamut
Update bigquery/google/cloud/bigquery/client.py
50ac8fe 
Co-Authored-By: Peter Lamut <plamut@users.noreply.github.com>
@tswast
Copy link
Contributor Author

tswast commented Jan 24, 2020

There are still a lot of client.dataset() calls in snippets.py, but that file is deprecated, IIRC.

That's right. We're moving those samples to the samples/ directory and updating them to avoid the use of client.dataset(). I'm hoping that with #10175 and #10176 we'll have enough snippets updated.

plamut
plamut approved these changes Jan 27, 2020
View reviewed changes
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

Looks good now. 👍

@plamut plamut merged commit 7e36fda into googleapis:master Jan 27, 2020
@tswast tswast deleted the bq-dot-dataset-deprecation branch January 27, 2020 16:22
@yoshi-automation yoshi-automation mentioned this pull request Jan 29, 2020
Merged
This was referenced Jan 29, 2020
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Closed
@yoshi-automation yoshi-automation mentioned this pull request Feb 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plamut plamut plamut approved these changes

@shollyman shollyman Awaiting requested review from shollyman

Assignees
No one assigned
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.

BigQuery: deprecate client.dataset()
5 participants
@tswast @busunkim96 @plamut @googlebot @yoshi-automation