Hi,
Looks good.
I would want to verify that it is defined such that if it later needs to be
abstracted up to ChronoLocalDate and apply to compatible Chronologies
and respective local dates such as JapaneseDate there is no conflict.
It should not be an issue since Period implements ChronoPeriod.
In the respective implementations, the estimation/computation of the
number of steps
would need to depend on the Chronology's definition of months.
I can sponsor this enhancement.
Thanks, Roger
On 1/20/16 10:15 AM, Stephen Colebourne wrote:
I'm happy with the logic and specification of this proposal. I think it
will be a useful addition.
I'll let the Oracle team chime in to do a further review.
thanks
Stephen
On 16 January 2016 at 13:31, Tagir F. Valeev <amae...@gmail.com> wrote:
Hello!
Thanks for review! Here's the updated patch:
http://cr.openjdk.java.net/~tvaleev/webrev/8146218/r2/
SC> The docs say that if the end date is before the start date, the
SC> stream is empty. While I can see just about why LongStream.range()
SC> works that way, I don't think this API should. The minimum is an
SC> exception, but it would be easy to support negative in the
SC> days-only case or the months-only case. The problem is where there
SC> are both months/years and days and those should be exceptions.
Now datesUntil(endExclusive) throws an IllegalArgumentException if end
date is before start date.
datesUntil(endExclusive, step) supports negative periods. It throws
IllegalArgumentException if:
- step is zero
- step.toTotalMonths() and step.getDays() both non-zero and have
opposite sign
- step is negative and end date is after start date
- step is positive and end date is before start date
Otherwise it works correctly: you can use
LocalDate.of(2016, 1, 1)
.datesUntil(LocalDate.of(2015, 1, 1), Period.ofMonths(-1));
Steps like Period.of(-1, -1, -1) are also supported.
SC> The single-arg implementation uses plusDays() with an
SC> incrementing number. When the performance patch goes in, the
SC> proposed streaming implementation will be optimal for small
SC> streams but less optimal for large ones. The fastest way to loop
SC> over a list of dates would be to manually generate them by
SC> incrementing the day until it exceeds the length of month, and so
SC> on. However, this would be serial.
As I understand, plusDays performance patch is already pushed.
It's possible to write custom Spliterator which would use
previous.plusDays(1) in tryAdvance() and jump (via
LocalDate.of(startEpochDay+n)) in trySplit() if parallel processing is
requested. However this adds at least one additional class and more
complexity. I guess, such optimization can be considered later as
separate issue when API is stabilized.
SC> As such, I think the best way to write this, taking account of
SC> how plusDays() is implemented, is as follows:
SC> LongStream.range(start.toEpochDay(),
SC> end.toEpochDay()).mapToObj(LocalDate::ofEpochDay);
Ok, now it's done this way.
SC> In the period-based method, it would be best to capture the case
SC> where totalMonths == 0 and days > 0 and forward to another method
SC> that ignores months. That private method can share implementation
SC> with the public single-arg method (passing in 1).
This case still more complex than one-day case as it requires division
and multiplication. Thus I'd keep these case separately. However I
simplified "months = 0" path using ofEpochDay, now it should be
faster.
SC> The docs for the period-based method should specify the strategy
SC> that produces the results (in abstract terms). This needs to cover
SC> that the result is equivalent to always adding to the start period
SC> a multiple of the period.
I added some clarifications, please check.
SC> Some nits:
SC> I prefer to avoid @link in the first sentence. Just using 'stream'
would be sufficient.
Done.
SC> The first sentence should be a summary. In this case it probably has a
bit too much detail.
Done.
SC> The @return has 'values' on a new line when it could be on the same
line.
I set now line length = 100 characters in my IDE. Is it ok?
SC> If statements need braces.
Done.
With best regards,
Tagir Valeev.
