-
Notifications
You must be signed in to change notification settings - Fork 20
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
Move datetime formatting to Datetime LogicalType #216
Conversation
Codecov Report
@@ Coverage Diff @@
## main #216 +/- ##
==========================================
- Coverage 99.86% 99.82% -0.05%
==========================================
Files 22 22
Lines 2218 2224 +6
==========================================
+ Hits 2215 2220 +5
- Misses 3 4 +1
Continue to review full report at Codecov.
|
@tamargrey I didn't review this in detail, but we will also want to update the new config options guide in the docs to remove the references to the global datetime format config variable. We will probably also want to include some documentation on how to work with logical types that have options specified, but that can be a separate issue and PR (we will also need this for Ordinal and other logical types that will eventually take options) |
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.
Just minor things. Looks good overall.
closes #198
Removes
'datetime_format'
from config and adds adatetime_format
to theDatetime
logical type so that any different formats can be indicated for each column.