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 a NaivePeriod data structure. #384

Closed
wants to merge 1 commit into from

Conversation

jwir3
Copy link

@jwir3 jwir3 commented Jan 4, 2020

NOTE: This is a work in progress, it's not quite ready to be merged yet I don't think.

A NaivePeriod covers the period of time between two NaiveDateTime(s),
inclusive. This data structure can be used to intersect two NaivePeriods to
determine if they overlap.

This is in partial fulfillment of #380, but does not provide necessary code
to handle DateTime objects (with offsets).

Refs #380.

A NaivePeriod covers the period of time between two NaiveDateTime(s),
inclusive. This data structure can be used to intersect two NaivePeriods to
determine if they overlap.

This is in partial fulfillment of chronotope#380, but does not provide necessary code
to handle DateTime objects (with offsets).

Refs chronotope#380.
@@ -440,7 +440,7 @@ pub use oldtime::Duration;
#[cfg(feature="clock")]
#[doc(no_inline)] pub use offset::Local;
#[doc(no_inline)] pub use offset::{TimeZone, Offset, LocalResult, Utc, FixedOffset};
#[doc(no_inline)] pub use naive::{NaiveDate, IsoWeek, NaiveTime, NaiveDateTime};
#[doc(no_inline)] pub use naive::{NaiveDate, IsoWeek, NaiveTime, NaiveDateTime, NaivePeriod};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm tempted to keep this out of the root namespace and put the period module behind an unstable feature flag for a month or two before it's released with back-compat guarantees. I might change my mind much faster than that, though, or be convinced otherwise.

Copy link
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

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

This looks great overall, a couple requests about reducing the public API, though!

And if you could add yourself and this PR to the changelog that'd be nice.

@@ -455,7 +455,7 @@ pub mod prelude {
#[cfg(feature="clock")]
#[doc(no_inline)] pub use Local;
#[doc(no_inline)] pub use {Utc, FixedOffset};
#[doc(no_inline)] pub use {NaiveDate, NaiveTime, NaiveDateTime};
#[doc(no_inline)] pub use {NaiveDate, NaiveTime, NaiveDateTime, NaivePeriod};
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here keeping it out of the prelude at least at the beginning.

Comment on lines +18 to +22
pub start: NaiveDateTime,

/// The date at which the period ends. This is inclusive, meaning that the period runs up to
/// and including this date/time.
pub end: NaiveDateTime
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep these private, and require a constructor.

/// As such, if you need to interset two time periods that are of differing time zones, you're out
/// of luck (patches accepted, though).
#[derive(PartialEq, Eq, PartialOrd, Ord, Copy, Clone, Debug)]
pub struct NaivePeriod {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we accomplish a bit more future compatibility and smaller api surface with a Period<T: Datelike + Timelike>? I'd rather reduce the number of places that chrono distinguishes between Naive and tz-aware types. Putting this behind an unstable flag will allow a punt on this for awhile.

Comment on lines +47 to +48
pub fn new(start: NaiveDateTime, end: NaiveDateTime) -> Self {
NaivePeriod { start: start, end: end }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it reasonable for us to verify that start < end or start <= end? Maybe rename to left, right and just put them in the right order?


use std::cmp::{min, max};

/// A period of time between two ISO 8601 dates/times ([NaiveDateTime](NaiveDateTime)s).
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw I think docs links only work on nightly, and docs.rs builds on nightly, so all links could be changed from the [..](..) to be just [..]. Separately, stylistically, I slightly prefer all docs links to just be simple names with backticks: e.g.

[`NaiveDateTime`]

@djc djc deleted the branch chronotope:master August 29, 2022 08:43
@djc djc closed this Aug 29, 2022
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.

3 participants