- Notifications
You must be signed in to change notification settings - Fork322
feat: add type hint for public methods#445
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| google/cloud/bigquery_v2/ | ||
| output = .pytype/ | ||
| # Workaround for https://github.com/google/pytype/issues/150 | ||
| disable = pyi-error |
HemangChothaniDec 22, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
tswast left a comment
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.
| fromgoogle.cloud.bigquery.tableimportTableReference | ||
| fromgoogle.cloud.bigquery.tableimportRowIterator | ||
| # 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 putQueryJob 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.
Whydict here andDict below?
| 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 idDict
python-bigquery/tests/system.py
Line 1645 indbc68b3
| self.assertEqual(set(response["permissions"]),set(permissions)) |
plamut commentedJan 21, 2021
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). |
plamut commentedApr 15, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Superseded by#613. (will also address the comments there) |
Fixes#157