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.

Sign up for GitHub

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

Jump to bottom

Add DateTime::<Utc>::from_timestamp_opt #1278

Closed
wants to merge 1 commit into from
Closed

Add DateTime::<Utc>::from_timestamp_opt #1278

wants to merge 1 commit into from

Conversation

demurgos
Copy link
Contributor

@demurgos demurgos commented Sep 9, 2023

This commit adds the new constructor from_timestamp_opt to build a DateTime<Utc> from a UNIX timestamp.

Figuring out how to convert a timestamp into a DateTime<Utc> was a common issue:

This commit should make DateTime<Utc> creation more discoverable and intuitive.

This commit respects the current convention of using the _opt suffix for fallible functions. As panicking variants on invalid input are deprecated, no panicking variant is provided. See this issue for discussion about error handling and panics.

Closes #832

@demurgos demurgos mentioned this pull request Sep 9, 2023
Open
@codecov
Copy link

codecov bot commented Sep 9, 2023 •
edited

Codecov Report

Merging #1278 (990a4e3) into main (cef9016) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1278      +/-   ##
==========================================
- Coverage   86.32%   86.30%   -0.03%     
==========================================
  Files          35       35              
  Lines       13896    13900       +4     
==========================================
  Hits        11996    11996              
- Misses       1900     1904       +4
Files Changed Coverage Δ
src/datetime/mod.rs 71.65% <0.00%> (-0.58%) ⬇️
src/lib.rs 40.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@demurgos
Copy link
Contributor Author

demurgos commented Sep 9, 2023 •
edited

The discussion in #832 proposed to add DateTime::from_timestamp, but since then there was the update deprecating panicking functions. That's why I only added from_timestamp_opt. I expect this to become from_timestamp if/once chrono error handling is refactored. Until then, _opt felt better for consistency.

Note that doctests were added, but Codecov seems to not count them.

@demurgos demurgos force-pushed the issue-815-from_timestamp_opt branch 4 times, most recently from a3ae3b8 to 0e52353 Compare September 10, 2023 00:06
@demurgos
Add DateTime::<Utc>::from_timestamp_opt
990a4e3 
This commit adds the new constructor `from_timestamp_opt` to build a `DateTime<Utc>` from a UNIX timestamp.

Figuring out how to convert a timestamp into a `DateTime<Utc>` was a common issue:
- chronotope#88
- chronotope#200
- chronotope#832

This commit should make `DateTime<Utc>` creation more discoverable and intuitive.

This commit respects the current convention of using the `_opt` suffix for fallible functions. As panicking variants on invalid input are deprecated, no panicking variant is provided. See [this issue](chronotope#815) for discussion about error handling and panics.

Closes chronotope#832
pitdicker
pitdicker requested changes Sep 10, 2023
View reviewed changes
Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

Can you please rebase on 0.4.x? Then it can be included in a 0.4 release, instead of a far-off 0.5.
That should also take care of code coverage with doctests.

src/datetime/mod.rs
/// invalid nanosecond, otherwise returns `Some(DateTime {...})`.
///
/// If you need to create a `DateTime` with some other [`TimeZone`], use [`TimeZone::timestamp_opt`]
/// or [`DateTime::with_timezone`].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good documentation 👍

src/datetime/mod.rs
#[must_use]
pub fn from_timestamp_opt(secs: i64, nsecs: u32) -> Option<Self> {
NaiveDateTime::from_timestamp_opt(secs, nsecs)
.map(|ndt| DateTime::from_naive_utc_and_offset(ndt, Utc))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be simplified to NaiveDateTime::from_timestamp_opt(secs, nsecs).map(|ndt| ndt.and_utc())

src/lib.rs
@@ -301,7 +301,7 @@
//!
//! ### Conversion from and to EPOCH timestamps
//!
//! Use [`Utc.timestamp(seconds, nanoseconds)`](./offset/trait.TimeZone.html#method.timestamp)
//! Use [`DateTime::from_timestamp_opt(seconds, nanoseconds)`](./struct.DateTime.html#method.from_timestamp_opt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is pre-existing, but can you replace the html link with:

[`DateTime::from_timestamp_opt(seconds, nanoseconds)`](DateTime::from_timestamp_opt)

@pitdicker
Copy link
Collaborator

This commit respects the current convention of using the _opt suffix for fallible functions.

For new methods I think we just drop the _opt suffix.

@demurgos
Copy link
Contributor Author

demurgos commented Sep 10, 2023 •
edited

Thank you for the quick review, I'll apply the changes and open a new PR for the branch 0.4.x. I also learned about the [`DateTime::from_timestamp_opt(seconds, nanoseconds)`](DateTime::from_timestamp_opt) syntax for links so it's nice :)

@demurgos demurgos mentioned this pull request Sep 10, 2023
Merged
@demurgos
Copy link
Contributor Author

demurgos commented Sep 10, 2023 •
edited

Superseded by the PR for the branch 0.4.x : #1279

@demurgos demurgos closed this Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitdicker pitdicker pitdicker requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Utc.timestamp method is unintuitive
2 participants
@demurgos @pitdicker