Consolidate handling of datetime columns #755
Conversation
frances-h
requested review from
amontanez24 and
R-Palazzo
and removed request for
a team
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #755 +/- ##
==========================================
+ Coverage 95.43% 95.47% +0.04%
==========================================
Files 112 112
Lines 4379 4357 -22
==========================================
- Hits 4179 4160 -19
+ Misses 200 197 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Looks good!
I looked for all the pd.to_datetime we're doing in the library and I'm pasting them here to decide if we should use _convert_datetime_column in those situations:
SDMetrics/sdmetrics/timeseries/base.py
Line 69 in f26cfc7
…with datetime_format fails
|
|
||
| # Run and Assert | ||
| with pytest.raises(ValueError, match="Column 'visits' is not a valid datetime"): | ||
| expected_msg = re.escape("Error converting column 'visits' to timestamp: ") |
There was a problem hiding this comment.
Should the actual error be included in the message?
There was a problem hiding this comment.
It is, but since it's a pandas error I didn't want to have the test fail if the message ever gets updated. Instead I just have the test match the initial substring we add.
Resolve #741