NotificationsYou must be signed in to change notification settings
Fork321
Star786

fix: add types to DatasetReference constructor#1601

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

Jump to bottom

Merged

tswast merged 13 commits intogoogleapis:mainfromkserruys:fix/datasetreference-constructor-types

Apr 12, 2024

Merged

fix: add types to DatasetReference constructor#1601

tswast merged 13 commits intogoogleapis:mainfromkserruys:fix/datasetreference-constructor-types

Apr 12, 2024

Conversation

Copy link

kserruys commentedJun 29, 2023•
edited
Loading

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:

Make sure to open an issue as abug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes#1598 🦕

fix: add types to DatasetReference constructor

ebf6952

product-auto-labelbot added size: xs

Pull request size is extra small.

api: bigqueryIssues related to the googleapis/python-bigquery API. labels

Jun 29, 2023

kserruys marked this pull request as ready for review

June 29, 2023 09:33

kserruys requested review froma team ascode owners

June 29, 2023 09:33

kserruys requested a review fromjainsahab

June 29, 2023 09:33

chalmerlowe added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Jun 29, 2023

chalmerlowe previously approved these changes

Jun 29, 2023

View reviewed changes

Copy link

Collaborator

chalmerlowe left a comment

Choose a reason for hiding this comment

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

LGTM

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Jun 29, 2023

Copy link

Collaborator

chalmerlowe commentedJun 29, 2023

Welp:

We appear to be faced with a mismatch. Doing a run of mypy against the code produces this error:

google/cloud/bigquery/dataset.py:187: error: Argument 1 to "DatasetReference" has incompatible type "Optional[str]"; expected "str"

Feel free to dig into this and see what produces that incompatible type on line 187 and what we might need to do to resolve this issue.

chalmerlowe dismissed theirstale review

June 29, 2023 18:24

tests failed

Merge branch 'main' into fix/datasetreference-constructor-types

06f689b

tswast added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Jun 30, 2023

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Jun 30, 2023

karelserruys-foodpairing added2 commits

July 6, 2023 13:56

fix: add types to DatasetReference constructor

ed2097f

Merge branch 'fix/datasetreference-constructor-types' ofhttps://gith…

b20a5c1

…ub.com/kserruys/python-bigquery into fix/datasetreference-constructor-types

product-auto-labelbot added size: sPull request size is small. and removed size: xsPull request size is extra small. labels

Jul 6, 2023

kserruys requested a review fromchalmerlowe

July 12, 2023 13:01

chalmerlowe added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Jul 13, 2023

yoshi-kokoro removed kokoro:force-runAdd this label to force Kokoro to re-run the tests. labels

Jul 13, 2023

Copy link

Collaborator

chalmerlowe commentedJul 13, 2023

@kserruys let's run kokoro and see what kinda results we get from the tests.

Copy link

Collaborator

chalmerlowe commentedJul 14, 2023

@kserruys

The two types ofprerelease dependency test failures are a known issue that I need to tackle and we can safely ignore them in this case.

TheKokoro test failure relates to the overall test coverage.
It looks like enough has changed in that portion of code by adding a new conditional branch that the Python librarycoverage feels our tests are not comprehensive enough.

Are you comfortable diving into the test suite to see what needs to be added to the tests to provide coverage for the new conditional?

chalmerloweand others added3 commits

July 14, 2023 08:07

Merge branch 'main' into fix/datasetreference-constructor-types

0090f77

fix: DatasetReference.from_string test coverage

008a317

Merge branch 'main' into fix/datasetreference-constructor-types

38f2fbc

kserruys commented

Aug 21, 2023

View reviewed changes

google/cloud/bigquery/dataset.py

Comment on lines -180 to +182

		eliflen(parts)>2:
		else:

Copy link

Author

kserruysAug 21, 2023

Choose a reason for hiding this comment

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

Hereelse: covers the same cases aselif len(parts) > 2:

Parameterdataset_id is a string, because of that listparts will always have at least 1 item.
By usingelse we can reassurepytest-cov that everything is ok.

Copy link

Author

kserruys commentedAug 21, 2023

@kserruys
The two types ofprerelease dependency test failures are a known issue that I need to tackle and we can safely ignore them in this case.
TheKokoro test failure relates to the overall test coverage. It looks like enough has changed in that portion of code by adding a new conditional branch that the Python librarycoverage feels our tests are not comprehensive enough.
Are you comfortable diving into the test suite to see what needs to be added to the tests to provide coverage for the new conditional?

@chalmerlowe

Sure :). I just took a while to find some time to do this.
Please have a look at the updated code. Coverage on my local machine return 100% now.

Regards

Merge branch 'main' into fix/datasetreference-constructor-types

e07cc59

meredithslota added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Sep 1, 2023

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Sep 1, 2023

meredithslota added2 commits

September 21, 2023 19:03

Merge branch 'main' into fix/datasetreference-constructor-types

bc71921

Merge branch 'main' into fix/datasetreference-constructor-types

347427d

meredithslota added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Nov 8, 2023

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Nov 8, 2023

Merge branch 'main' into fix/datasetreference-constructor-types

269dcd6

tswast added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Apr 12, 2024

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Apr 12, 2024

tswastenabled auto-merge (squash)

April 12, 2024 17:19

Merge branch 'main' into fix/datasetreference-constructor-types

abfcc94

tswast added the kokoro:runAdd this label to force Kokoro to re-run the tests. label

Apr 12, 2024

tswast approved these changes

Apr 12, 2024

View reviewed changes

yoshi-kokoro removed the kokoro:runAdd this label to force Kokoro to re-run the tests. label

Apr 12, 2024

tswast added the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Apr 12, 2024

Merge branch 'main' into fix/datasetreference-constructor-types

e23cf4b

tswast added kokoro:force-runAdd this label to force Kokoro to re-run the tests. and removed kokoro:force-runAdd this label to force Kokoro to re-run the tests. labels

Apr 12, 2024

yoshi-kokoro removed the kokoro:force-runAdd this label to force Kokoro to re-run the tests. label

Apr 12, 2024

tswast merged commitbf8861c intogoogleapis:main

Apr 12, 2024