Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.

fix: annotate optional integer parameters with optional type - #1487

Merged
tswast merged 4 commits into
mainfrom
tswast-type-annotations
Feb 8, 2023
Merged

fix: annotate optional integer parameters with optional type#1487
tswast merged 4 commits into
mainfrom
tswast-type-annotations

Conversation

@tswast

@tswasttswast commented Feb 7, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

I noticed in a project that uses this client that the type annotations for result are incorrect.

bigframes/dataframe.py:64: error: Argument "max_results" to "result" of "QueryJob" has incompatible type "Optional[int]"; expected "int" [arg-type]

Fixes#509

@tswast
tswast requested review from a team and yirutangFebruary 7, 2023 15:53
@product-auto-labelproduct-auto-labelBot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/python-bigquery API. labels Feb 7, 2023
@product-auto-labelproduct-auto-labelBot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Feb 7, 2023
@chalmerlowe

chalmerlowe commented Feb 8, 2023

Copy link
Copy Markdown
Collaborator

General question:

what was the criteria for deciding which parameters should get the enhanced Type Annotation?

Noticed some other int parameters in various methods that did not get the same enhancement and was curious.

chalmerlowe
chalmerlowe approved these changes Feb 8, 2023
View reviewed changes

@chalmerlowechalmerlowe left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a general question to the comment thread.
Beyond that question, this LGTM.

@tswast

tswast commented Feb 8, 2023

Copy link
Copy Markdown
ContributorAuthor

what was the criteria for deciding which parameters should get the enhanced Type Annotation?
Noticed some other int parameters in various methods that did not get the same enhancement and was curious.

Our approach was: when you touch a method, add type annotations.

At the time, that meant we could also remove the type information from the docstring because it was then redundant information from what Sphinx generated and what we put in the docstring. Looks to be the same with https://cloud.google.com/python/docs/reference/bigquery/latest/google.cloud.bigquery.client.Client but I'm not 100% sure how the linking works over there since they don't use intersphinx.

@tswast
tswast merged commit a190aaa into mainFeb 8, 2023
@tswast
tswast deleted the tswast-type-annotations branch February 8, 2023 16:05
@release-pleaserelease-pleaseBot mentioned this pull request Feb 8, 2023
Merged
This was referenced Feb 28, 2023
Closed
Closed
Closed
abdelmegahedgoogle pushed a commit to abdelmegahedgoogle/python-bigquery that referenced this pull request Apr 17, 2023
@tswast@abdelmegahedgoogle
fix: annotate optional integer parameters with optional type (googlea…
dd21845
…pis#1487)
* fix: annotate optional integer parameters with optional type
* remove google-cloud-core reference causing type checker issues
deps: update minimum google-cloud-core to 1.6.0
@tswasttswast mentioned this pull request Nov 6, 2023
Closed
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@yirutangyirutangAwaiting requested review from yirutang
1 more reviewer
@chalmerlowechalmerlowechalmerlowe approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: bigqueryIssues related to the googleapis/python-bigquery API.size: mPull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

increase minimum version of google-cloud-core to 1.6.0

2 participants

@tswast@chalmerlowe