-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
ENH: infer Timestamp(iso8601string) resolution #49737
Conversation
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.
From a first pass looks good, there's just a couple of merge conflicts to solve
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.
Looks pretty good. Do we have any tests where the iso string has a UTC offset?
im pretty sure we have loads of these, unless we're thinking of different things. you mean like "2016-01-01 04:05:06-01:00" right? |
Yeah exactly. Should those test assert that the resolution is a certain value too? |
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 (I think the failure are unrelated) Merge when ready @MarcoGorelli
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.
Looks good, I've just left some questions I'd like to double-check before merging myself - sorry if they're basic, I'm just a bit concerned about the bus factor on some of this work
Not blockers though, feel free to merge without my approval here
obj.value = tz_localize_to_utc_single( | ||
value, obj.tzinfo, ambiguous=None, nonexistent=None, creso=reso | ||
) | ||
obj.creso = reso |
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.
this is for use by ensure_reso
?
return convert_to_tsobject(ival, tz, None, False, False) | ||
ival = tz_localize_to_utc_single( | ||
ival, tz, ambiguous="raise", nonexistent=None, creso=reso | ||
) | ||
obj = _TSObject() | ||
obj.dts = dts | ||
obj.value = ival | ||
obj.creso = reso | ||
maybe_localize_tso(obj, tz, obj.creso) | ||
return obj |
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.
is this because convert_to_tsobject
doesn't (yet?) allow passing a reso
to set before calling maybe_localize_tso
?
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.
some combination of that and me not wanting to recurse. honestly i dont remember how much of each off the top of my head
elif ( | ||
isinstance(tz, tzlocal) | ||
and is_platform_windows() | ||
and _offset in (QuarterEnd, BQuarterBegin, BQuarterEnd) | ||
): | ||
request.node.add_marker( | ||
pytest.mark.xfail(reason="After GH#49737 t.tzinfo is None on CI") | ||
) |
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.
I'm confused - the xfail reason only mentions t.tzinfo
, and the if-statement condition depends on _offset
, yet t = Timestamp("20080101", tz=tz)
doesn't depend on _offset
, why's that?
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.
best guess is some kind of windows-themed demon trying to increase my blood pressure. seriously i couldn't reproduce it so just punted.
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.
OK, no objections. I don't think I've ever seen anyone use tz_local
anyway
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.
Looks good to me pending green (I may not be awake by the time CI finishes, but no objections to this being merged)
looks good - merging then, thanks @jbrockmendel ! |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.