-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: is_temporal should be overridden by is_dttm value #11429
Conversation
bff1a24
to
578f9a6
Compare
Codecov Report
@@ Coverage Diff @@
## master #11429 +/- ##
==========================================
- Coverage 66.55% 61.90% -4.65%
==========================================
Files 860 861 +1
Lines 40866 40920 +54
Branches 3694 3694
==========================================
- Hits 27198 25332 -1866
- Misses 13570 15408 +1838
- Partials 98 180 +82
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
makes sense
SUMMARY
TableColumn
has three properties to check the datatype of a column:is_numeric
,is_string
andis_temporal
.is_temporal
is usually referenced when a new dataset is added to populate default values foris_dttm
, which indicates whether or not a column is to added to the time column dropdown. While we should usually directly referenceis_dttm
, the other slightly ambiguously named property can sometimes be used, causing incorrect behaviour.This PR changes the behaviour of
is_temporal
to first check the value ofis_dttm
, and if thatis not None
(=after initial metadata extraction), return that value, otherwise check the datatype based on the column type. The placeholder text is also fixed, as it featured non-compliant syntax (Year is%Y
, not%y
).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Added test.
ADDITIONAL INFORMATION