Skip to content

Adding ability to give default QueryJobConfig to Client - #6088

Merged
tswast merged 11 commits into
googleapis:masterfrom
marketdial:master
Sep 26, 2018
Merged

tswast merged 11 commits into
googleapis:masterfrom
marketdial:master

Conversation

@blainehansen

Copy link
Copy Markdown
Contributor

closes #5183

This isn't a complete request (I need to do things like fill out the Contributor License Agreements, get coverage all the way to 100%, etc), but I wanted to begin the discussion to see if this was an acceptable direction. Only QueryJobConfig has been implemented, but the other types shouldn't be too difficult, since I've added a _JobConfig.fill_from_default method.

@googlebot

Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Sep 24, 2018
@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Sep 24, 2018
@tseaver

tseaver commented Sep 25, 2018

Copy link
Copy Markdown
Contributor

@blainehansen

Making a PR from your master branch makes things hard for you over the long term: it creates barriers to synchronizing with this repository's master over time.

WRT the test failures, there is missing line / branch coverage for:

google/cloud/bigquery/client.py               374      1    160      1    99%   1244, 1236->1244

which is the case where no job_config is passed.

@blainehansen

Copy link
Copy Markdown
Contributor Author

@tseaver Sorry about not creating a feature branch ha, I don't expect to maintain a fork for a long period, I was just hoping to include this feature, but I shouldn't have cut corners.

@tseaver
tseaver requested a review from tswast September 25, 2018 20:43
Comment thread bigquery/google/cloud/bigquery/client.py Outdated
def query(
self, query, job_config=None, job_id=None, job_id_prefix=None,
self, query,
job_config=None, override_job_config=False,

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Comment thread bigquery/tests/unit/test_job.py Outdated
Comment thread bigquery/google/cloud/bigquery/job.py Outdated
Comment thread bigquery/google/cloud/bigquery/job.py Outdated
Comment thread bigquery/google/cloud/bigquery/job.py Outdated

@tswast tswast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good, thanks! Just a few comments. I'm not convinced we need the merge_job_config argument yet.

Comment thread bigquery/google/cloud/bigquery/client.py Outdated
Comment thread bigquery/google/cloud/bigquery/job.py Outdated
Comment thread bigquery/google/cloud/bigquery/job.py Outdated

@tswast tswast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Love it! Thank you so much for your contribution, @blainehansen.

FYI: I've made fill_from_default "private" by prefixing an underscore, because I don't think it's necessary to add this method to the public API interface. (It's fine to use from other modules within this library.)

@tseaver Please take a look.

@blainehansen

Copy link
Copy Markdown
Contributor Author

👍 yeah making that private is definitely a better choice 😄

@tswast
tswast merged commit cf04f6d into googleapis:master Sep 26, 2018
@blainehansen

Copy link
Copy Markdown
Contributor Author

🎊 🎊 🎊

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BigQuery: ability to set default job / table / dataset configs in client class

4 participants