-
Notifications
You must be signed in to change notification settings - Fork 304
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 hint for public methods #445
Conversation
google/cloud/bigquery_v2/ | ||
output = .pytype/ | ||
# Workaround for https://github.com/google/pytype/issues/150 | ||
disable = pyi-error |
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.
This file is synthtool generated so changes are not allowed here, but for the reference and pass current tests changed the current file.
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.
google/pytype#150 is marked as closed. Can you provide a little more context as to why this is needed?
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.
Looking good, thanks! A few questions.
@@ -75,6 +75,18 @@ | |||
from google.cloud.bigquery.table import TableReference | |||
from google.cloud.bigquery.table import RowIterator | |||
|
|||
# Types needed only for Type Hints |
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 don't see any reason to keep these in a separate location from the other imports. Let's put them in their respective groups.
max_creation_time=None, | ||
): | ||
project: str = None, | ||
parent_job: Union[_AsyncJob, str] = None, |
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.
parent_job
is only relevant for queries, so I think we can put QueryJob
here and avoid the use of a private type.
table: Union[Table, TableReference, str], | ||
rows: Union[Sequence[Tuple], Sequence[Dict]], | ||
selected_fields: Sequence[SchemaField] = None, | ||
**kwargs: dict, |
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.
Why dict
here and Dict
below?
table: Union[Table, TableReference], | ||
permissions: Sequence[str], | ||
retry: retries.Retry = DEFAULT_RETRY, | ||
timeout: float = None, | ||
): |
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.
Looking at the system tests, I believe the output for this method id Dict
python-bigquery/tests/system.py
Line 1645 in dbc68b3
self.assertEqual(set(response["permissions"]), set(permissions)) |
Bump, just checking the status of this? Generally speaking, though, type hints would be welcome, even though static type checks have not been added to Kokoro (yet). |
Superseded by #613. (will also address the comments there) |
Fixes #157