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

implement Iterator #152

Open
Geobert opened this issue May 14, 2017 · 19 comments
Open

implement Iterator #152

Geobert opened this issue May 14, 2017 · 19 comments

Comments

@Geobert
Copy link

Geobert commented May 14, 2017

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.

@rnleach
Copy link

rnleach commented May 14, 2017 via email

@Geobert
Copy link
Author

Geobert commented May 14, 2017

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)
        }
    }
}

@lifthrasiir
Copy link
Contributor

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.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 10, 2018

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
Copy link
Contributor

gnzlbg commented Jan 16, 2018

So I have submitted a PR with an implementation of an Iterator over the days with a step of 1 day for NaiveDate: #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.

@withtypes
Copy link

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.

@igiagkiozis
Copy link

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?

@rnleach
Copy link

rnleach commented Jul 16, 2020

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?

@quodlibetor
Copy link
Contributor

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
Copy link
Contributor

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.

@withtypes
Copy link

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.

@quodlibetor
Copy link
Contributor

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.

@withtypes
Copy link

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
Copy link

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

@elbaro
Copy link

elbaro commented Aug 17, 2020

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()) { ... }

@brianbruggeman
Copy link

.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.

@withtypes
Copy link

@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).

@brianbruggeman
Copy link

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.

@withtypes
Copy link

@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.

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 a pull request may close this issue.