-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: Allow for to_datetime
/ strftime
to automatically parse dates with single-digit hour/minute/second
#20144
Conversation
thanks - can you add a test please? |
Thanks for reminding |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #20144 +/- ##
==========================================
+ Coverage 79.57% 79.63% +0.06%
==========================================
Files 1563 1564 +1
Lines 217395 217448 +53
Branches 2472 2473 +1
==========================================
+ Hits 172986 173169 +183
+ Misses 43841 43710 -131
- Partials 568 569 +1 ☔ View full report in Codecov by Sentry. |
@MarcoGorelli Is there anything else I can do? Take a look if you have a moment :) |
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.
Could you make a (tiny) update to the regex?
(Added a comment inline about it).
try_parse_dates
to parse dates with single-digit hour/minute
try_parse_dates
to parse dates with single-digit hour/minuteto_datetime
/ strftime
to automatically parse dates with single-digit hour/minute/second
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.
thanks @wsyxbcl
I think this should be good - leaving open a bit in case anyone else has objections, then will ship it (edit @alexander-beedie gonna need you to approve too since you requested changes)
To accommodate the default datetime format used by Excel in certain regional settings (e.g., China), as discussed in #16092
closes #16092
The format mentioned in the original issue is actually H:m, with seconds omitted. However, since the seconds component is already optional in the current pattern, adding support for single-digit seconds shouldn't cause any issues.