brick / date-time

Date and time library for PHP
MIT License
337 stars 30 forks source link

Add LocalDateRange::toDuration, YearMonthRange::toDuration methods #48

Closed solodkiy closed 1 year ago

solodkiy commented 2 years ago

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

BenMorel commented 2 years ago

Apart from my comment, LGTM :+1:

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 commented 2 years ago

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 commented 2 years ago

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

BenMorel commented 2 years ago

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

Also, Java's threeten-extra has a [toPeriod](https://www.threeten.org/threeten-extra/apidocs/org.threeten.extra/org/threeten/extra/LocalDateRange.html#toPeriod()) method, that we may replicate here; maybe we should name this one toDuration() for consistency then?

solodkiy commented 2 years ago

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 commented 2 years ago

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:

As for the logic behind this, what about:

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

solodkiy commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

solodkiy commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

What's your use case for this method exactly?

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