search

spatie / period

Complex period comparisons
https://spatie.be/open-source
MIT License
1.6k stars 72 forks source link

Allow extension of Period that forces extension of DateTimeImmutable #38

Closed winkbrace closed 5 years ago

winkbrace commented 5 years ago

Allow extension of Period that forces extension of DateTimeImmutable (like Carbon or Chronos)

With some annotations I could pretty simply extend Period to a version that only works with Chronos objects in my project. There was only one issue in Period where it forces its own class instead of the extended class.

The accompanying test looks pretty complex for such a small change, but it best explains the issue.

jeromegamez commented 5 years ago

Wouldn‘t the same be achieved by using DateTimeInterface instead of DateTimeImmutable in the constructor? Or am I missing something?

jeromegamez commented 5 years ago

Ah, I am missing the point :). You want to extend the Period, not just allow non-DateTimeImmubles. (I‘m on the road, and didn‘t grasp it on first mobile view)

winkbrace commented 5 years ago

Yes, sorry for the incomplete explanation. I want to extend Period to only use Chronos objects internally.

brendt commented 5 years ago

Great PR, just some minor code style remarks. If these can be fixed I'll merge it 👍

winkbrace commented 5 years ago

Updated PR. I hope it's ok now :)

brendt commented 5 years ago

Tagged as 1.4.2: https://github.com/spatie/period/releases/tag/1.4.2

Thanks for the contribution!