chrono icon indicating copy to clipboard operation
chrono copied to clipboard

implement Iterator

Open Geobert opened this issue 7 years ago • 19 comments

Hi, I'm new at Rust so I might have missed something here. I have two NaiveDate and I'd like to write:

for d in start..end {
// ...
}

but it needs the Iterator Trait.

Geobert avatar May 14 '17 09:05 Geobert

What time step to use? Days, Months, Years, Decades?

On Sun, May 14, 2017 at 3:55 AM, Geobert Quach [email protected] wrote:

Hi, I'm new at Rust so I might have missed something here. I have two NaiveDate and I'd like to write:

for d in start..end {

    }

but it needs the Iterator Trait.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/chronotope/chrono/issues/152, or mute the thread https://github.com/notifications/unsubscribe-auth/AJSF1pqzd0RcLQeXQ3LdPsanepC64qt5ks5r5s-dgaJpZM4NaVGx .

rnleach avatar May 14 '17 16:05 rnleach

oh, true, so the only choice is to do our own, maybe provide a factory to generate an iterator with needed parameters?

For my needs, I've done this:

pub struct NaiveDateIter {
    start: NaiveDate,
    end: NaiveDate
}

impl NaiveDateIter {
    pub fn new(start: NaiveDate, end: NaiveDate) -> NaiveDateIter {
        NaiveDateIter { start: start, end: end }
    }
}

impl Iterator for NaiveDateIter {
    type Item = NaiveDate;
    fn next(&mut self) -> Option<NaiveDate> {
        
        if self.start.gt(&self.end) {
            None 
        } else {
            let res = self.start;
            self.start = self.start.succ();
            Some(res)
        }
    }
}

Geobert avatar May 14 '17 19:05 Geobert

Daywise iterator for Range<NaiveDate> sounds not too bad (#139 suggests that we may need Week, so why not Month or Year :-). The problem is that it will need an implementation of std::iter::Step trait, which is currently unstable.

lifthrasiir avatar May 27 '17 13:05 lifthrasiir

I think implementing std::iter::Step for day-wise iteration of NaiveDate is "ok" and we can make it available to users of rust nightly only by putting it behind a feature flag.

Maybe a couple of methods could make this more explicit (let d: NaiveDate; let o: NaiveDate;):

  • d.days(): unbounded iterator from the date to the last representable date, allows d.days().take(100) to get the next 100 days
  • d.months(): same as above for months
  • d.years(): same as above for years
  • d.days_until(o): bounded iterator for the days in range [d, o).
  • d.months_until(o): same as above for months
  • d.years_until(o): same as above for years

To add these methods we don't really need to implement std::iter::Step.

gnzlbg avatar Jan 10 '18 11:01 gnzlbg

So I have submitted a PR with an implementation of an Iterator over the days with a step of 1 day for NaiveDate: https://github.com/chronotope/chrono/pull/209

It is an ExactSize iterator, and in the future it could implement Step like @lifthrasiir mentions, TrustedLen, and FusedIterator.

Once this iterator implements Step, users can iterate over weeks by just using a step size of 7 days. We might add an easy utility function for that at that point but I don't think it is worth it to do so now.

Iterating over day ranges is also trivial to add, you just do .iter_days() + take or take_while or take_until... to delimit the range. Since users can do this, I don't think we should add an utility function for this in the library, at least not right now.

Finally, iterating over months... I tried to do this, and succeeded, just to realize I had failed. I don't think that iterating over months should be in the library, because there is no good solution for it. All of the following solutions are good and valid, depending on the use case:

  • iterate over months with a constant step-size of 30 days or 4 weeks. At some point, you are going to get two consecutive dates in the same month, but such is life.

  • iterate from one month to the next ending up in the same day of the month and without consecutive dates in the same months. I implemented this because this is what C++ Boost DateTime library does. It is complicated. One needs to detect whether the initial date is the last date of a given month, store that in the iterator, and also store the initial day of the month in the iterator. Otherwise if the initial date is in the [29,31] day of the month, and you go through February, things get messed up.

  • do what .with_month() would do and if the initial date is in the [29,31] range of days of a month just directly skip to MAX_DATE and return None.

  • probably many other ways to do this, involving different trade-offs.

IMO none of these is wrong, but none of these solutions work for everybody either. We should maybe add some examples to the library showing how to implement these so that users can refer to them, but we should not add APIs to the library for any of these. None of them is better than another.

gnzlbg avatar Jan 16 '18 15:01 gnzlbg

time step to use? Days, Months, Years, Decades?

I think the smallest difference between naive dates (1 day) is very intuitive. NaiveDate provides a method succ and I would expect a range to go through successive values.

I'm new to Rust, when trying to implement IntoIterator for a range of dates, the compiler says "conflicting implementations of trait std::iter::IntoIterator for type std::ops::RangeInclusive<chrono::NaiveDate> conflicting implementation in crate core: impl<I> std::iter::IntoIterator for I where I: std::iter::Iterator;. I'm a little bit lost on how could for d in start..end in theory work.

withtypes avatar Oct 10 '18 08:10 withtypes

Hi, I think this feature request makes sense. Perhaps not as is, but being able to produce a range of dates easily is very useful. If there's no implementation already I can try my hand at this; any thoughts?

igiagkiozis avatar Jul 16 '20 20:07 igiagkiozis

Perhaps a similar method for NaiveDateTime would be useful? I imagine a method that takes a duration and end-time, or a number of steps, and produces an iterator. Would the maintainers be open to PRs along this line for NaiveDate and NaiveDateTime?

rnleach avatar Jul 16 '20 23:07 rnleach

I would be super happy to accept a PR implementing iterators!

I don't think a raw iter() method would make sense but having a few methods (e.g. iter_days(), iter_weeks(), iter_seconds()) that return customized iterators would be super helpful. I'd be happy for PRs that implement any subset of these methods for both NaiveDateTime/DateTime or just NaiveDate (no reason to implement anything on date).

If someone decides to implement this please make sure to test around some daylight savings time changes in both directions, it's fine if tests fail in the initial PR, we can discuss the right behavior in review.

quodlibetor avatar Jul 17 '20 15:07 quodlibetor

To be clear, I don't think that implementing IntoIterator makes a lot of sense until we've implemented iter_* methods and have experience that only one is ever used.

quodlibetor avatar Jul 17 '20 15:07 quodlibetor

I would be super happy to accept a PR implementing iterators!

I think @gnzlbg made PR #208 and described everything above.

Again, I don't understand why some people find it strange that canonical way to iterate over a range of dates should be by one day. That's the smallest step. Unfortunately, Step trait is not yet stable. I think implementing Step would make ranges automatically iterable (by one step—one day).

Iterating by month or year is less intuitive because, unlike weeks, months and years are not fixed-sized and depend on the year. What happens if you iterate by month/year starting from February 29. There is no obvious choice. Methods like start/end of (next/prev) month/year, are much more useful and explicit.

withtypes avatar Jul 20 '20 09:07 withtypes

Oh, yes they did. I've been working my way through the backlog, I'll get to that one asap, if they're still interested in getting it going.

Iterating by month or year is less intuitive because, unlike weeks, months and years are not fixed-sized and depend on the year. What happens if you iterate by month/year starting from February 29. There is no obvious choice. Methods like start/end of (next/prev) month/year, are much more useful and explicit.

The fact that the logic is more complex is part of the reason that you'd want to push it down into a library where fixes can be shared. Iterating over days is trivial because you can just iterate over integers added to a base day.

quodlibetor avatar Jul 20 '20 15:07 quodlibetor

The fact that the logic is more complex is part of the reason that you'd want to push it down into a library where fixes can be shared. Iterating over days is trivial because you can just iterate over integers added to a base day.

I didn't mean it's necessary complex, just ambiguous. For example, if you choose "always skip 30 days" for month iteration, as somebody mentioned above, then it's simple. My point was that there are different ways to "iterate by month" from an arbitrary date, so it's likely that a user will have a little bit different requirements.

In general, I don't think it has to do with complex/trivial. Iterating over a vec is also trivial (iterate over integers and use the index operator). What is useful are existing abstractions and patterns built on iterators.

withtypes avatar Jul 21 '20 06:07 withtypes

By the way, thanks for working on chrono! I'm just trying to be helpful!

withtypes avatar Jul 21 '20 06:07 withtypes

step_by_abc() can be useful too.

for date in start.date()..=end.date() { ... }
for date in (start.date()..=end.date()).step_by_month() { ... }
for time in (start.time()..=end.time()).step_by_duration(Duration::from_secs(..)) { ... }
for date in (start..end).step_by_duration(Duration::from_secs(..)).map(|dt| dt.date()) { ... }

elbaro avatar Aug 17 '20 19:08 elbaro

.step_by(duration: Duration) seems like it would fit nearly all of the use-cases. As a developer, you may not be able to conceive of a reason to step by a month or by a specific time (e.g. let's say 5 minutes), because you have not run across that use-case. There are definitely use-cases that none of us have thought about. As a library, the goal really should be to make the API flexible enough and robust enough that most use-cases can be collected in a small API. Having a bunch of step_by_* methods seems like api bloat with a heavier long-term maintenance cost.

I have a use case where I need to load a schedule into a cache and for fast API lookups, I need to explode a simple start/end date-time timestamp into individual entries for the length of the range at a specific granularity, defined by an environment variable. The environment variable lets us make different choices for development vs testing vs production based on memory constraints. At the moment, I'm stuck with building a custom iterator to do all of this, and on top of that, I'd really like for this iterator to be parallelized where it makes sense using rayon.

brianbruggeman avatar Dec 15 '20 19:12 brianbruggeman

@brianbruggeman note that the discussion was about NaiveDate, not timestamps (datetime). For DateTime it makes sense to have Duration steps, but for dates the smallest step is "day" (and as I said previously, I really find it natural to iterate over some discrete concept by the smallest unit-day).

withtypes avatar Dec 15 '20 19:12 withtypes

I don't think my point is invalidated by considering a NaiveDate rather than a Date+Time timestamp. I gave the timestamp as an example, but Duration could just as easily be days and weeks as it could be anything else. But consider that data is often segregated by different kinds of time, it seems like a developer might want to iterate over months and years or possibly decades. In my case, I care about "near" real time, so I'm less likely to be concerned about larger times. But from my perspective, Date is really just another kind of time and is lumped into the same type of problems and constraints from a usability perspective.

brianbruggeman avatar Dec 15 '20 19:12 brianbruggeman

@brianbruggeman sure, but then you need to define what it means to iterate NaiveDate by 1 minute. We don't have an API for iterating integers by 0.1 either. IMO, it's also not well defined what it means to iterate by anything other than days and weeks (because of leap years, unequal month durations, etc.), but it seems that other people disagree with this which is fine.

withtypes avatar Dec 15 '20 20:12 withtypes