-
Notifications
You must be signed in to change notification settings - Fork 309
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
feat: add type hints to Client #2044
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
google/cloud/bigquery/client.py
Outdated
if client_options is None: | ||
client_options = {} | ||
if isinstance(client_options, dict): | ||
client_options = google.api_core.client_options.from_dict(client_options) | ||
assert isinstance(client_options, google.api_core.client_options.ClientOptions) | ||
|
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 am curious, why did we choose to:
- move this content from after the call to
super()
to before the call tosuper()
? What is the reason for reordering this content? - add an assert statement when one was not used previously?
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.
- Moving this part before the call to
super()
is optional, but I did it because actually the exact same type check & coercion happens inside thesuper()
(Client.__init__
) as well, so I let it early break in the second time. - The reason for adding the assert statement is to let the type checker know the type of
client_options
, otherwise it wouldn't know that theif client_options.api_endpoint:
expression at the line 261 is valid .
@rinarakaki I ran the kokoro CI/CD checks. mypy errors:
unit test errors:
|
Moves import from being used solely during specific checks to being more universally available.
testing some minor changes to deal with mypy quirks
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 for providing this update to the code. |
* add type hints * Update client.py Moves import from being used solely during specific checks to being more universally available. * Update google/cloud/bigquery/client.py * Update client.py testing some minor changes to deal with mypy quirks * Update google/cloud/bigquery/client.py --------- Co-authored-by: Chalmer Lowe <chalmerlowe@google.com>
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕