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 Day<Tz> #883

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
313 changes: 313 additions & 0 deletions src/day.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,313 @@
use core::cmp::Ordering;
use core::convert::TryFrom;
use core::fmt;
use core::fmt::Debug;
use core::fmt::Display;
use core::fmt::Write;
use core::ops::Add;
use core::ops::Sub;

use crate::oldtime;
use crate::DateTime;
use crate::Datelike;
use crate::Days;
use crate::NaiveDate;
use crate::NaiveTime;
use crate::TimeZone;
use oldtime::Duration;

/// Represents the full range of local timestamps in the day
///
///
#[derive(Clone, Copy)]
pub struct Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
date: NaiveDate,
tz: Tz,
}

impl<Tz> Day<Tz>
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: would prefer to have the inherent impl before trait impls.

Also I would write impl<Tz: TimeZone + Copy + Display> instead of having the where clause`, since it's a bit more concise.

where
Tz: TimeZone + Copy + Display,
{
///
pub fn date(&self) -> NaiveDate {
self.date
}

///
pub fn zone(&self) -> Tz {
self.tz
}

///
pub fn new(date: NaiveDate, tz: Tz) -> Day<Tz> {
Day { date, tz }
}

/// Returns the earliest datetime on the given day
///
/// panics: This will panic in the following cases:
/// * There are no valid local times in the first six hours of the day
/// other: This will return the incorrect value when:
/// * There are valid local times not aligned to a 15-minute reslution
pub fn start(&self) -> DateTime<Tz> {
Copy link
Member

Choose a reason for hiding this comment

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

Could use some documentation of why we have to use a loop here. What problem is this trying to solve?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added some more documentation here including around panics and incorrect results

Copy link
Contributor

Choose a reason for hiding this comment

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

One way to have this not require heuristics would be to make the loop slightly differently:

  • first, try with NaiveTime::MIN as a fast path
  • then, for each 15min segment,
    • if MIN + segment_end is not ok, go to the next segment
    • else, if MIN + segment_end - 1ns is ok too, do a dichotomy between MIN + segment_begin and MIN + segment_end

That said, I personally don't think that doing this would be necessary. Summer time changes across midnight are unlikely enough already, having them not be a multiple-of-15-min offset would be actively harmful to basically any software.

Another idea would be, to delegate the implementation of this to a new trait method on TimeZone, that would have a default implementation of adding NaiveTime::MIN; and on the day chrono_tz gets a summer time transition across midnight the specialization can be added there.

I'm personally partial towards the second idea, as it gets us all of the simplest implementation right now, the most efficient one, and the most future-proof one. Does that make sense?

// All possible offsets: https://en.wikipedia.org/wiki/List_of_UTC_offsets
// means a gap of 15 minutes should be reasonable

// while looping here is less than ideal, in the vast majority of cases
// the inital start time guess will be valid. We have to loop here because
// we don't know ex-ante what the earliest valid local time on the day will be
// and so we have to attempt a number of reasonable local times until we find one.
//
// Reasonable in this case means that this function will work for all timezones
// mentioned in the above wikipedia list, but will panic in custom implementations
// that allow offsets not aligned to a 15-minute resolution.

let base = NaiveTime::MIN;
for multiple in 0..=24 {
let start_time = base + oldtime::Duration::minutes(multiple * 15);
match self.tz.from_local_datetime(&self.date.and_time(start_time)) {
crate::LocalResult::None => continue,
crate::LocalResult::Single(dt) => return dt,
// in the ambiguous case we pick the one which has an
// earlier UTC timestamp
// (this could be done without calling `naive_utc`, but
// this potentially better expresses the intent)
crate::LocalResult::Ambiguous(dt1, dt2) => {
if dt1.naive_utc() < dt2.naive_utc() {
return dt1;
} else {
return dt2;
}
}
}
}

panic!("Unable to calculate start time for date {} and time zone {}", self.date, self.tz)
}

/// Returns the exclusive end date of the day, equivalent to the start of the next day
///
/// Returns None when it would otherwise overflow
pub fn exclusive_end(&self) -> Option<DateTime<Tz>> {
self.checked_add_days(Days::new(1))?.start().into()
}

///
pub fn checked_add_days(self, days: Days) -> Option<Self> {
if days.0 == 0 {
return Some(self);
}

i64::try_from(days.0).ok().and_then(|d| self.diff_days(d))
}

///
pub fn checked_sub_days(self, days: Days) -> Option<Self> {
if days.0 == 0 {
return Some(self);
}

i64::try_from(days.0).ok().and_then(|d| self.diff_days(-d))
}

fn diff_days(self, days: i64) -> Option<Self> {
let date = self.date.checked_add_signed(Duration::days(days))?;
Some(Day { date, ..self })
}
Comment on lines +99 to +120
Copy link
Member

Choose a reason for hiding this comment

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

I feel like in general we should start with a minimal API here, we can grow it over time as people actually ask for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

}

impl<Tz> Datelike for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
fn year(&self) -> i32 {
self.date.year()
}

fn month(&self) -> u32 {
self.date.month()
}

fn month0(&self) -> u32 {
self.date.month0()
}

fn day(&self) -> u32 {
self.date.day()
}

fn day0(&self) -> u32 {
self.date.day0()
}

fn ordinal(&self) -> u32 {
self.date.ordinal()
}

fn ordinal0(&self) -> u32 {
self.date.ordinal0()
}

fn weekday(&self) -> crate::Weekday {
self.date.weekday()
}

fn iso_week(&self) -> crate::IsoWeek {
self.date.iso_week()
}

fn with_year(&self, year: i32) -> Option<Self> {
Some(Self { date: self.date.with_year(year)?, tz: self.tz })
}

fn with_month(&self, month: u32) -> Option<Self> {
Some(Self { date: self.date.with_month(month)?, tz: self.tz })
}

fn with_month0(&self, month0: u32) -> Option<Self> {
Some(Self { date: self.date.with_month0(month0)?, tz: self.tz })
}

fn with_day(&self, day: u32) -> Option<Self> {
Some(Self { date: self.date.with_day(day)?, tz: self.tz })
}

fn with_day0(&self, day0: u32) -> Option<Self> {
Some(Self { date: self.date.with_day0(day0)?, tz: self.tz })
}

fn with_ordinal(&self, ordinal: u32) -> Option<Self> {
Some(Self { date: self.date.with_ordinal(ordinal)?, tz: self.tz })
}

fn with_ordinal0(&self, ordinal0: u32) -> Option<Self> {
Some(Self { date: self.date.with_ordinal0(ordinal0)?, tz: self.tz })
}
}

impl<Tz> Debug for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
Debug::fmt(&self.date, f)?;
f.write_char(' ')?;
self.tz.fmt(f)
}
}

impl<Tz> Display for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
Debug::fmt(self, f)
}
}

impl<Tz> PartialOrd for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
self.date.partial_cmp(&other.date)
}
}

impl<Tz> Ord for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
fn cmp(&self, other: &Self) -> Ordering {
self.date.cmp(&other.date)
}
}

impl<Tz> PartialEq for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
fn eq(&self, other: &Self) -> bool {
self.date.eq(&other.date)
}
}

impl<Tz> Eq for Day<Tz> where Tz: TimeZone + Copy + Display {}

impl<Tz> Add<Days> for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
type Output = Day<Tz>;

fn add(self, days: Days) -> Self::Output {
self.checked_add_days(days).unwrap()
}
}

impl<Tz> Sub<Days> for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
type Output = Day<Tz>;

fn sub(self, days: Days) -> Self::Output {
self.checked_sub_days(days).unwrap()
}
}

impl<Tz> From<DateTime<Tz>> for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
fn from(dt: DateTime<Tz>) -> Self {
Day { date: dt.date_naive(), tz: dt.timezone() }
}
}

#[cfg(test)]
mod tests {
use super::Day;
use crate::Utc;

#[test]
fn test_start_time() {
assert_eq!(
Day::from(Utc::now()).start(),
Utc::now()
.date_naive()
.and_hms_opt(0, 0, 0)
.unwrap()
.and_local_timezone(Utc)
.single()
.unwrap(),
);
}
}

#[cfg(feature = "serde")]
#[cfg_attr(docsrs, doc(cfg(feature = "serde")))]
mod serde {
use crate::{Day, TimeZone};
use core::fmt::Display;
use serde::ser;

// Currently no `Deserialize` option as there is no generic way to create a timezone
// from a string representation of it. This could be added to the `TimeZone` trait in future

impl<Tz> ser::Serialize for Day<Tz>
where
Tz: TimeZone + Copy + Display,
{
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: ser::Serializer,
{
serializer.collect_str(&self)
}
}
}
3 changes: 3 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,9 @@ mod date;
#[allow(deprecated)]
pub use date::{Date, MAX_DATE, MIN_DATE};

mod day;
pub use day::Day;

mod datetime;
#[cfg(feature = "rustc-serialize")]
#[cfg_attr(docsrs, doc(cfg(feature = "rustc-serialize")))]
Expand Down