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

NaiveDateTime to DateTime helper method #711

Merged
merged 7 commits into from Jun 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -32,6 +32,7 @@ Versions with only mechanical changes will be omitted from the following list.
* Fix panicking when parsing a `DateTime` (@botahamec)
* Add support for getting week bounds based on a specific `NaiveDate` and a `Weekday` (#666)
* Remove libc dependency from Cargo.toml.
* Add the `and_local_timezone` method to `NaiveDateTime`

## 0.4.19

Expand Down
18 changes: 17 additions & 1 deletion src/naive/datetime/mod.rs
Expand Up @@ -21,7 +21,7 @@ use crate::naive::date::{MAX_DATE, MIN_DATE};
use crate::naive::time::{MAX_TIME, MIN_TIME};
use crate::naive::{IsoWeek, NaiveDate, NaiveTime};
use crate::oldtime::Duration as OldDuration;
use crate::{Datelike, Timelike, Weekday};
use crate::{DateTime, Datelike, LocalResult, TimeZone, Timelike, Weekday};

#[cfg(feature = "rustc-serialize")]
pub(super) mod rustc_serialize;
Expand Down Expand Up @@ -722,6 +722,22 @@ impl NaiveDateTime {
pub fn format<'a>(&self, fmt: &'a str) -> DelayedFormat<StrftimeItems<'a>> {
self.format_with_items(StrftimeItems::new(fmt))
}

/// Converts the `NaiveDateTime` into the timezone-aware `DateTime<Tz>`
/// with the provided timezone, if possible.
///
/// This is experimental and might be removed in the future. Feel free to
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we calling this experimental again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was proposed in this comment. Apparently this API gets reinvented a lot. #711 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think labeling this as experimental isn't helpful because it's not clear what downstream users should do with that information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a really good point - happy to set up a PR to remove this line if you think that makes the most sense.

I'd noticed this elsewhere in the codebase, so figured there was some precedent for mentioning something as experimental/unstable - albeit this actually shows up in the readme so has a higher probability of actually getting feedback.

Another way to improve this is I could try to co-ordinate somewhere to actually get the feedback and provide a link in the docs (this could be interesting for other changes as well) - potentially github discussions on this repo could be an option, or just issues?

Copy link
Contributor

@djc djc Jun 20, 2022

Choose a reason for hiding this comment

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

A Cargo feature is useful there because it includes extra dependencies. Also IMO "unstable" (as opposed to "experimental") has a more or less well-known meaning, in that semver-stable updates can break that particular API. Including that in the default features is probably not great, since it's too easy to miss the documentation. So I think the precedent from unstable-locales isn't all that applicable here.

We could link to an issue or enable GitHub discussions, but it feels like overkill to me in this case -- it seems to me that this API is a decent addition that "paves a cowpath" in a manner consistent with other existing API.

In short, yes, IMO we could just remove the comment about this being experimental.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @djc - that makes sense, I've submitted #715 to rectify this.

/// let us know what you think about this API.
///
/// # Example
///
/// ```
/// use chrono::{NaiveDate, Utc};
/// let dt = NaiveDate::from_ymd(2015, 9, 5).and_hms(23, 56, 4).and_local_timezone(Utc).unwrap();
/// assert_eq!(dt.timezone(), Utc);
pub fn and_local_timezone<Tz: TimeZone>(&self, tz: Tz) -> LocalResult<DateTime<Tz>> {
tz.from_local_datetime(self)
}
}

impl Datelike for NaiveDateTime {
Expand Down
15 changes: 14 additions & 1 deletion src/naive/datetime/tests.rs
@@ -1,7 +1,7 @@
use super::NaiveDateTime;
use crate::naive::{NaiveDate, MAX_DATE, MIN_DATE};
use crate::oldtime::Duration;
use crate::Datelike;
use crate::{Datelike, FixedOffset, Utc};
use std::i64;

#[test]
Expand Down Expand Up @@ -240,3 +240,16 @@ fn test_nanosecond_range() {
NaiveDateTime::from_timestamp(nanos / A_BILLION, (nanos % A_BILLION) as u32)
);
}

#[test]
fn test_and_timezone() {
let ndt = NaiveDate::from_ymd(2022, 6, 15).and_hms(18, 59, 36);
let dt_utc = ndt.and_local_timezone(Utc).unwrap();
assert_eq!(dt_utc.naive_local(), ndt);
assert_eq!(dt_utc.timezone(), Utc);

let offset_tz = FixedOffset::west(4 * 3600);
let dt_offset = ndt.and_local_timezone(offset_tz).unwrap();
assert_eq!(dt_offset.naive_local(), ndt);
assert_eq!(dt_offset.timezone(), offset_tz);
}