-
Notifications
You must be signed in to change notification settings - Fork 35
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
Fixes for IntoValue for chrono::DateTime<Utc> and chrono::DateTime<FixedOffset> #115
Conversation
77233f6
to
4891f21
Compare
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 for this!
I've added time_timespec_new
, which takes an offset, so you should be able to use that to avoid funcall
.
CHANGELOG.md
Outdated
- Bug fixes for `IntoValue` for `chrono::DateTime<Utc>` and | ||
`chrono::DateTime<FixedOffset>` |
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.
You can skip this, the changelog only needs to describe differences from the last release.
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.
Sounds good.
I noticed some of my checkpoint commits made it into main
("Reviewer comments", etc.). Should I be amending and force pushing to my topic branch to keep a single atomic commit before the PR is merged? Or perhaps you prefer non-atomic commits in main. Let me know if I should do anything specific here.
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 not that fussed about how other people structure their commits. I'll merge the PR with the default "Create a merge commit" option on GitHub. If you want a chance to get commits how you like before I merge, just let me know and I can hold off on merging until you give me the ok.
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.
Should be good to go now, apart from any additional changes you'd like.
4891f21
to
e44f580
Compare
} else { | ||
Err(Error::new( | ||
Ruby::get_with(val).exception_arg_error(), | ||
"time must not be negative", | ||
"time nanos must not be negative", | ||
)) |
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.
While we're here, I took the liberty of adding support for converting Ruby Time
objects for dates prior to 1970 to SystemTime
. I believe Ruby's normalization of timespecs will make it unlikely that we'll get a timespec
with negative nanoseconds, but I'm not confident of 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.
Now that you've added support for taking a timespec and providing an offset, a question that occurs to me is whether a Perhaps not, since a Time
object created from a SystemTime
should start out in UTC instead of the system timezone.SystemTime
represents a clock on a specific system, even though Time
is timezone aware.
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 agree that it seems extremely unlikely that we'll get a timespec
with negative nanos, but in this case I don't think there's any harm in having the error handling there just in case.
The way I think about it, Time is a value (seconds from epoch), and a label on how to interpret it (the zone). Like how a string is a value (some bytes) and a label (the encoding).
With my way of thinking about it SystemTime is just a value, without the label (like [u8]
vs String
). Ruby's Time doesn't have the option to omit the label, so what's the default label?
I'd probably go with UTC being the sensible default (but there might be some bias there having grown up in England basically on the prime meridian). However I think there's a pretty strong case for Ruby having picked local time as the default, so I think we should stick with local.
9d06816
to
02d93c9
Compare
02d93c9
to
3ad1e9d
Compare
A recent PR added support for converting to and from
chrono::DateTime<Utc>
andchrono::DateTime<FixedOffset>
. This PR does the following:Time
objects are created in the system's local timezone rather than the timezone of thechrono
datetime.Due to my lack of knowledge, I have resorted to going through
funcall
, but I suspect there's a way to do this more efficiently through C API.