-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Propagate name in more cases when using to_datetime
#21848
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
Conversation
@@ -250,8 +243,11 @@ def _convert_listlike_datetimes(arg, box, format, name=None, tz=None, | |||
result, timezones = array_strptime( | |||
arg, format, exact=exact, errors=errors) | |||
if '%Z' in format or '%z' in format: | |||
return _return_parsed_timezone_results( | |||
result, timezones, box, tz) | |||
tz_results = _return_parsed_timezone_results( |
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.
missing return here actually
#21703 is also proposed for this issue, though its currently failing. So happy to have you add tests. |
I was trying to stick to the convention in the file, but wanted is there a reason for local imports? Is that the convention in the codebase or is it OK to float the imports to the top?
|
Codecov Report
@@ Coverage Diff @@
## master #21848 +/- ##
==========================================
+ Coverage 91.92% 91.92% +<.01%
==========================================
Files 160 160
Lines 49916 49917 +1
==========================================
+ Hits 45885 45886 +1
Misses 4031 4031
Continue to review full report at Codecov.
|
@eyurtsev you should not use local imports except in very specific circumstances |
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.
needs tests. also you are changing some extraneous code, can you limit changes to what is needed to solve the issue.
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.
needs tests and a whatsnew note
closing as stale. would still take this PR if updated. |
Closes #21697
Haven't ran tests yet