search

brick / date-time

Date and time library for PHP
MIT License
323 stars 29 forks source link

add toDatePeriod method for LocalDateRange #31

Closed morrislaptop closed 3 years ago

morrislaptop commented 3 years ago

Adds a toDatePeriod to the LocalDateRange object which returns a native \DatePeriod PHP object.

I opted to do end of day (instead of midnight next day) to be inline with the inclusive nature of LocalDateRange.

Note: This is not bricks/date-time Period class - I hope this doesn't create confusion.

BenMorel commented 3 years ago

Hi, this can be a great addition indeed.

Are you sure we shouldn't use the next day though, for the end date?

Otherwise the last date is not included when you iterate the DatePeriod object, which makes it behave differently from LocalDateRange in this regard: https://3v4l.org/5Zggu

morrislaptop commented 3 years ago

Hi, this can be a great addition indeed.

Are you sure we shouldn't use the next day though, for the end date?

Otherwise the last date is not included when you iterate the DatePeriod object, which makes it behave differently from LocalDateRange in this regard: https://3v4l.org/5Zggu

Hmmm I've added some assertions to verify the iterator and they seem to match up exactly. Looking into it further.

morrislaptop commented 3 years ago

Ah yep, got you now. I feel like the essence of the library is inclusive, whereas \DatePeriod is exclusive. So there's a mismatch of ideas here.

What do you think users would expect?

In my scenario I just want to grab the end of the range in a DateTime context and stick it in a database.

Perhaps it might be better to do toEndDateTime() and toStartDateTime() which respect the inclusive nature of this library.

toDatePeriod would be exclusive which would respect the exclusive nature of the native object.

BenMorel commented 3 years ago

Could this be configured with a flag?

public function toDatePeriod(bool $nextDay)

I'm afraid there isn't an ideal solution anyway, as you said there's a mismatch of ideas between brick's LocalDateRange and DatePeriod.

morrislaptop commented 3 years ago

Actually, I don't see the issue here. Both objects produce the same result when iterating.

The example you provided had the end date at the start of the day, whereas this method sets the end date at the end of the day.

The native DatePeriod will include the last day, even if it's one second past the start of the day.

This is what the latest version of the tests ensure.

E.g. https://3v4l.org/WSEnO

I think it's good to go as is :)

morrislaptop commented 3 years ago

Hey @BenMorel , do you think you'll be merging this? I've implemented something similar over at https://github.com/spatie/period/pull/81

BenMorel commented 3 years ago

The native DatePeriod will include the last day, even if it's one second past the start of the day.

I missed this detail, thanks. One more gotcha with native DateTime classes ;-)

Please fix the few points I mentioned and it should be good to go.

BenMorel commented 3 years ago

Thank you. Released as 0.2.3!