-
Notifications
You must be signed in to change notification settings - Fork 309
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: support empty array in _from_api_repr_struct #2010
base: main
Are you sure you want to change the base?
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
a290f75
to
54e3a59
Compare
@TommyDew42 Thank you for suggesting a code improvement. It does appear that we may need to make our code logic more robust in that area. Right now, this PR does not appear to have a matching test. The
A test similar to test_from_api_repr_w_struct_type, but with an empty With one or more test cases to exercise the logic, we will also need to ensure that If you are not sure how to proceed with producing tests, let us know, we might be able to offer some guidance. |
from google.cloud.bigquery.query import ( | ||
ScalarQueryParameterType, | ||
StructQueryParameterType, | ||
) |
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.
Why do we do import within each test instead of at the top of the test module?
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.
And i believe these import changes are from running nox -s blacken
and nox -s lint
.
self.assertEqual( | ||
str(param.array_type), | ||
str( | ||
StructQueryParameterType( | ||
ScalarQueryParameterType("STRING", name="name"), | ||
ScalarQueryParameterType("INT64", name="age"), | ||
) | ||
), | ||
) |
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.
It doesn't work to do assertEqual(param.array_type, StuctQueryParameterType(Sclar...)
. Not sure what the better way would be.
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 #2009 🦕