-
Notifications
You must be signed in to change notification settings - Fork 308
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
fix: add types to DatasetReference constructor #1601
fix: add types to DatasetReference constructor #1601
Conversation
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.
LGTM
Welp: We appear to be faced with a mismatch. Doing a run of mypy against the code produces this error:
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. |
…ub.com/kserruys/python-bigquery into fix/datasetreference-constructor-types
@kserruys let's run kokoro and see what kinda results we get from the tests. |
The two types of The 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? |
elif len(parts) > 2: | ||
else: |
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.
Here else:
covers the same cases as elif len(parts) > 2:
Parameter dataset_id
is a string, because of that list parts
will always have at least 1 item.
By using else
we can reassure pytest-cov
that everything is ok.
Sure :). I just took a while to find some time to do this. Regards |
Thanks @kserruys so much for the contribution and for your patience on this! |
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:
Fixes #1598 🦕