Add LocalDateRange::toDuration, YearMonthRange::toDuration methods

[solodkiy]

No description provided.

[solodkiy]

Also we could add this method to YearMonthRange (looks reasonable), and Year, YearMonth, YearWeek (not so sure).

[BenMorel]

Apart from my comment, LGTM 👍

Could you please consider adding LocalDateRange::getPeriod() as well in another PR? Although that one is less straightforward, as we have 2 ways to convert it to a period: calculating x years, y months & z days, or just 0 years, 0 months, and n days. Maybe we should even offer both ways? Let's start the discussion in #51.

YearMonthRange::getPeriod() would make sense, but not getDuration() as you cannot convert it to a number of days.

As for Year, YearMonth, YearWeek, they're not ranges, so I don't see how this could translate to a Duration or Period?

[solodkiy]

YearMonthRange::getPeriod() would make sense, but not getDuration() as you cannot convert it to a number of days.

Why not? YearMonthRange is inclusive. It include full first month, everything in between and full last month.
So I think YearMonthRange include all days between first day of first range month and last day of last range month (include corner days).
This approach allow us to convert YearMonthRange to LocalDateRange and to Duration.

[solodkiy]

Year is not really a range, but also could represent a whole year from first day to last.

[BenMorel]

I see. Let's start with this one and discuss the others in another PR.

Also, Java's threeten-extra has a toPeriod method, that we may replicate here; maybe we should name this one toDuration() for consistency then?

[solodkiy]

maybe we should name this one toDuration() for consistency then?

I am not sure we should fight for consistency with Java here.

Can you describe main logic of choosing between to and get methods?

[BenMorel]

I have no idea, but this project started as port of Java's ThreeTen API, so whenever we can keep the same API, I think we should do it for two reasons:

• there are many people behind this initiative, so they may have had this discussion before, and may have come to this consensus for a reason; to quote another contributor's comment:

  (...) we can't possibly replicate the collective brainpower and empirical evidence that went into designing JSR 310 and so, where practical, I would suggest simply copying JSR 310's API

• this can make this project less confusing for people coming from a Java background

As for the logic behind this, what about:

• get*() for fields that are part of the object (getYear(), getMonth(), ...)
• to*() for objects that are not part of it, but can be derived from it? (toDuration())

Though I'm sure you can find a counter-example in the codebase.

[solodkiy]

Though I'm sure you can find a counter-example in the codebase.

I will look for it, maybe it good point to do some rename for consistency then.

[BenMorel]

I finally chose toPeriod() in #51 for consistency with threeten-extra, so please let's use toDuration() here for consistency.

Though I'm sure you can find a counter-example in the codebase.

I will look for it, maybe it good point to do some rename for consistency then.

Thank you, that'd be great.

[solodkiy]

@BenMorel I renamed method, and also added YearMonthRange::toDuration

[BenMorel]

Shouldn't this be Duration::ofDays($this->count() - 1)?

As it stands, toPeriod() returns a 0-day Period if the start date is equal to the end date (this is in line with its Java counterpart).

So wouldn't it be weird if toDuration() returned a 1-day Duration for, say, 2022-06-05/2022-06-05?

[solodkiy]

This object is iterable and countable: the iterator returns all the LocalDate objects contained
in the range, while count() returns the total number of dates contained in the range.

The end date, inclusive.

I think duration of one day range is One day, not zero)

[solodkiy]

As it stands, toPeriod() returns a 0-day Period if the start date is equal to the end date (this is in line with its Java counterpart).

Quite weird. Is LocalDateRange in java inclusive too?

[BenMorel]

I think it makes sense in terms of Period: there is 1 day between 2022-06-06 an 2022-06-07.

That is: LocalDate::parse('2022-06-06')->plusPeriod(Period::ofDays(1)) is 2022-06-07, so this approach is consistent.

In terms of Duration, maybe the approach is different. But this can be confusing.

[solodkiy]

I think about time zones and Local Range duration. In some time zones with Daylight Saving Time duration between day start and day end is not so obvious.

For example Europe/London:

Sunday, 27 March 2022, 01:00:00 clocks were turned forward 1 hour to
Sunday, 27 March 2022, 02:00:00 local daylight time instead.

So Duration of range 27 March 2022 to 28 March 2022 in London is 47h.
Should we allow pass TimeZone param to toDuration() method?

[BenMorel]

I agree that you need the timezone if you want to introduce a toDuration() method, as 24 hours per day would be an oversimplification here.

Maybe that's the reason why threeten-extra doesn't provide this method at all.

What's your use case for this method exactly?

[solodkiy]

What's your use case for this method exactly?

Actually I don't remember it anymore :)
I will return when I do.