Fix: Update type hints for various BigQuery files#2206
Conversation
This commit addresses Issue #2132 by updating type hints in the following files: - google/cloud/bigquery/external_config.py - google/cloud/bigquery/job/base.py - google/cloud/bigquery/routine/routine.py - google/cloud/bigquery/schema.py - google/cloud/bigquery/table.py These changes improve code clarity and maintainability by providing more accurate type information.
product-auto-label
bot
added
size: s
shollyman
left a comment
shollyman
left a comment
There was a problem hiding this comment.
Mostly just questions about whether things should be more permissive of optional fallbacks.
| prop = self._properties.get("schema", {}) # type: ignore | ||
| return [SchemaField.from_api_repr(field) for field in prop.get("fields", [])] # type: ignore | ||
| prop: Dict[str, Any] = typing.cast( | ||
| Dict[str, Any], self._properties.get("schema", {}) |
There was a problem hiding this comment.
Should this be Optional?
There was a problem hiding this comment.
Since this method call has a default value:
If you call .get() you will get back the schema dict OR you will get back an empty dict, so I don't think Optional applies here.
| # TODO: The typehinting for this needs work. Setting this pragma to temporarily | ||
| # manage a pytype issue that came up in another PR. See Issue: #2132 | ||
| return self._properties["projectId"] # pytype: disable=typed-dict-error | ||
| return self._properties.get("projectId", "") |
There was a problem hiding this comment.
Should these be optional / return none as well?
There was a problem hiding this comment.
All of these (project, dataset_id, routine_id) are used as directly as inputs to a string formatting function. So setting them to None seems slightly more risky than setting them to an empty string. i.e. they should not exist at all (their default state OR be some form of string (empty OR otherwise)).
@property
def path(self):
"""str: URL path for the routine's APIs."""
return "/projects/%s/datasets/%s/routines/%s" % (
self.project,
self.dataset_id,
self.routine_id,
)
If we used None, path() could end up producing the following:
"/projects/None/datasets/None/routines/None"
Which might produce undesirable path collisions (not likely, but possible)
Whereas with an empty string we would get:
"/projects//datasets//routines/"
Should result in an error upon receipt by the back end.
3 participants
This commit addresses several elements of Issue #2132 by updating type hints in the following files:
These changes improve code clarity and maintainability by providing more accurate type information.
Partially fixes #2132 🦕