-
Notifications
You must be signed in to change notification settings - Fork 0
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
support to Client.
#19
Conversation
@@ -141,6 +142,9 @@ class Client(ClientWithProject): | |||
requests. If ``None``, then default info will be used. Generally, | |||
you only need to set this if you're developing your own library | |||
or partner tool. | |||
client_options (google.api_core.client_options.ClientOptions or dict): | |||
(Optional) Client options used to set user options on the client. | |||
API Endpoint should be set through client_options. |
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 have a bad feeling about this.
Can't find in docs how should I write the description with several class types
. Just listed them with or
, as it shown in previous versions.
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.
Quoting an already merged PR that addressed the same issue:
:type client_options: :class:`~google.api_core.client_options.ClientOptions` or :class:`dict`
:param client_options: (Optional) Client options used to set user options on the client.
API Endpoint should be set through client_options.
Hope this helps.
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.
Yeah, i saw that. But these are different types of docs. That's why i assumed to put just or
here.
@@ -141,6 +142,9 @@ class Client(ClientWithProject): | |||
requests. If ``None``, then default info will be used. Generally, | |||
you only need to set this if you're developing your own library | |||
or partner tool. | |||
client_options (google.api_core.client_options.ClientOptions or dict): | |||
(Optional) Client options used to set user options on the client. | |||
API Endpoint should be set through client_options. |
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.
Quoting an already merged PR that addressed the same issue:
:type client_options: :class:`~google.api_core.client_options.ClientOptions` or :class:`dict`
:param client_options: (Optional) Client options used to set user options on the client.
API Endpoint should be set through client_options.
Hope this helps.
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.
I checked this test and compared it to the example PR and as I see at client_options
should be something more specific, but not only DEFAULT_API_ENDPOINT
. That's why I suppose to bring back "https://foo-translation.googleapis.com"
statement, to check constructor with it.
@mf2199 could you please check it out one more time for me and confirm my thoughts?
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.
So far I do not see much more to it. As long as it works and passes the tests, it should be OK for now. The client_options
can always be updated in the future, if so desired.
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.
So far I do not see much more to it. As long as it works and passes the tests, it should be OK for now. The client_options
can always be updated in the future, if so desired.
client_options
support to Client.client_options
support to Client.
Changing the description of the class.
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.
It seems to be written off from Bu Sun Kim's PR, so I think it's okay
@emar-kar, let's start with this one. When we'll understand, that everything is ok, we'll create PPRs for others in row |
* 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
Addresses #8475
Some nox tests/unittests/pytests still failing on my PC. According to the trace its not the fault of my chgs.