Skip to content
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

Add direct constructors for OffsetDateTime. #641

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

jcdyer
Copy link
Contributor

@jcdyer jcdyer commented Dec 14, 2023

Fixes #640

This PR adds new constructors to OffsetDateTime:

  • OffsetDateTime::new_in_offset(date: Date, time: Time, offset: UtcOffset) -> Self
  • OffsetDateTime::new_utc(date: Date, time: Time) -> Self

Rationale for the PR can be found on the linked issue.

@jcdyer
Copy link
Contributor Author

jcdyer commented Dec 14, 2023

Can someone help me interpret the output of the coverage build, and understand what actions I should take? To my eye, it looks like a failed snapshot in a part of the codebase I didn't touch. Let me know if I'm missing something, and how I can resolve it.

Copy link
Member

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about code coverage; the output is what I expect. It's currently slightly broken due to a bug in the compiler that I need to open an issue for. However, tests for these methods would be appreciated.

For the API itself, I would prefer not to have now_local. It would be quite error prone given that UTC offsets change throughout the year in most of the western world. new_utc is good as-is, though I'd like to see new renamed to new_in_offset for clarity.

time/src/offset_date_time.rs Outdated Show resolved Hide resolved
time/src/offset_date_time.rs Outdated Show resolved Hide resolved
@jcdyer jcdyer requested a review from jhpratt December 18, 2023 15:26
@jcdyer
Copy link
Contributor Author

jcdyer commented Dec 18, 2023

I tend to prefer new over new_in_offset, as I think it is clear enough from the required arguments that it includes the offset, and this is the most natural constructor for an OffsetDateTime. But that's a minor quibble, and I'm happy to do it the way you think makes the most sense.

Changes made as requested, tests added, and ready for a new review.

@jhpratt
Copy link
Member

jhpratt commented Dec 19, 2023

I just realized that this is only implemented in offset_date_time.rs and not in date_time.rs. Given that I'm reworking some of the internals in the latter at the moment, I'll handle that.

This PR looks good. Merging!

@jhpratt jhpratt merged commit b77ed50 into time-rs:main Dec 19, 2023
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OffsetDateTime lacks an obvious constructor
2 participants