-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
BigQuery: Add client_options
to constructors for manual clients.
#8999
BigQuery: Add client_options
to constructors for manual clients.
#8999
Conversation
Changing the description of the class.
client_options
to constructors for manual clients.
bigquery/tests/unit/test_client.py
Outdated
project=self.PROJECT, | ||
credentials=creds, | ||
_http=http, | ||
client_options={"api_endpoint": Connection.DEFAULT_API_ENDPOINT}, |
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.
Because this test is for default Client constructor arguments, let's omit client_options
from this test, but still check that API_BASE_URL
has the expected value.
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 that we need a different test where we set client_options
as a dictionary. Let's add such a test and set a value for api_endpoint
to something other than the default value.
bigquery/tests/unit/test_client.py
Outdated
|
||
creds = _make_credentials() | ||
http = object() | ||
client_options = ClientOptions(Connection.DEFAULT_API_ENDPOINT) |
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 check with an alternative value for the api_endpoint
, since we are trying to test for non-default values in this test.
* added new test to check 'client_options' as a dict * deleted extra asserts * chged object at 'test_ctor_w_client_options_object' to an alternative value
* fixed lint issues
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.
Thanks for updating the tests, just one question.
Toward #8475