-
Notifications
You must be signed in to change notification settings - Fork 322
feat: Adds attributes to SchemaField #2077
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
feat: Adds attributes to SchemaField #2077
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 |
| INTERVAL = "INTERVAL" # NOTE: not available in legacy types | ||
| RANGE = "RANGE" # NOTE: not available in legacy types | ||
| FOREIGN = "FOREIGN" # NOTE: type acts as a wrapper for data types | ||
| # not natively understood by BigQuery unless translated |
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.
nit: can this be indented to align with the rest of the commend above it?
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.
Interesting.
When you try to indent the second line, black the Python formatter kicks the starting point back to the left and aligns it under the F in Foreign.
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.
I moved both lines of comment above the object FOREIGN so that they both align together.
google/cloud/bigquery/enums.py
Outdated
| DATETIME = "DATETIME" | ||
| INTERVAL = "INTERVAL" # NOTE: not available in legacy types | ||
| RANGE = "RANGE" # NOTE: not available in legacy types | ||
| FOREIGN = "FOREIGN" # NOTE: type acts as a wrapper for data types |
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.
Is there a test that could cover using this new enum?
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.
I supplemented the existing test that walks through some of enums with this enum in particular.
| """Definition of the foreign data type. | ||
| Only valid for top-level schema fields (not nested fields). | ||
| If the type is FOREIGN, this field is required. |
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.
are there any checks for this?
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.
There is now. 😺 Starting around line 250.
Adds several new attributes to
SchemaFieldand adds to the existing test suite to account for the new attributes.