-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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(ingest/bigquery): Fix BigQueryTableType enum accesses #7685
fix(ingest/bigquery): Fix BigQueryTableType enum accesses #7685
Conversation
@@ -19,7 +18,7 @@ | |||
logger: logging.Logger = logging.getLogger(__name__) | |||
|
|||
|
|||
class BigqueryTableType(Enum): | |||
class BigqueryTableType: |
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.
we could still use StrEnum?
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.
Saw this, don't think it'll work here unless I change the queries -- that's what's actually failing, where I directly embed BigQueryTableType.MATERIALIZED_VIEW
etc. in query text. If I keep as an Enum, then I have to change to BigQueryTableType.MATERIALIZED_VIEW.value
, but I didn't see any value in keeping it as an enum when it's basically just a string const holder.
In [2]: from enum import Enum
In [3]: class Mine(str, Enum):
...: A = "ONE"
...: B = "TWO"
...:
In [4]: Mine.A
Out[4]: <Mine.A: 'ONE'>
In [5]: Mine.A == "ONE"
Out[5]: True
In [6]: str(Mine.A)
Out[6]: 'Mine.A'
In [7]: str(Mine.A.value)
Out[7]: 'ONE'
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.
ugh didn't realize that it overrode the __str__
yep let's leave it like this then
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #7685 +/- ##
==========================================
- Coverage 74.87% 66.55% -8.33%
==========================================
Files 353 353
Lines 35386 35385 -1
==========================================
- Hits 26496 23550 -2946
- Misses 8890 11835 +2945
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 80 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@@ -464,7 +463,7 @@ def test_get_views_for_dataset( | |||
last_altered=bigquery_view_1.last_altered, | |||
comment=bigquery_view_1.comment, | |||
view_definition=bigquery_view_1.view_definition, | |||
table_type=BigqueryTableType.VIEW, | |||
table_type="VIEW", |
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 this change needed even after removing Enum subclassing from BigqueryTableType
class ?
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.
this is test, then its okay.
Checklist