-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
[viz flow] detect TIMESTAMP, transition to line chart #5634
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5634 +/- ##
==========================================
+ Coverage 63.44% 63.45% +0.01%
==========================================
Files 360 360
Lines 22953 22958 +5
Branches 2558 2557 -1
==========================================
+ Hits 14562 14568 +6
+ Misses 8376 8375 -1
Partials 15 15
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.
LGTM, just a comment on determining date... I feel there's a lot of logic there to detect a date, and it's split in two places. Might be good moving it to a single method.
superset/dataframe.py
Outdated
is_date = False | ||
if col_db_type: | ||
is_date = self.is_date(self.df.dtypes[col]) or any([ | ||
col_db_type.lower().startswith(s) for s in ('time', 'data') |
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.
Should we move this extra logic to the is_date
method? Eg:
def is_date(self, dtypes, col_db_type=None):
if col_db_type and any(col_db_type.lower().startswith(s) for s in ('time', 'data')):
return True
...
is_date = self.is_date(self.df.dtypes[col], col_db_type.lower())
02ecdfc
to
0e44da6
Compare
0e44da6
to
e014717
Compare
Good call, refactored all this. |
* [viz flow] detect TIMESTAMP, transition to line chart * Refactor is_date
No description provided.