Make Period iterable

[jeromegamez]

I'm using this PR to propose a new feature, and at the same time ask a question about the the road that you envision this library (which I, by the way, like a lot ❤️) to go forward, because it is related to further contributions that I have in the pipeline as follow ups ^^

I am using Period in a project with the aim to replace the DatePeriod class where possible. To be able to do this, I'm missing the ability to iterate over a a Spatie period as I would with the native class.

That's why I would like make a Period iterable with the changes proposed in this PR, so that this would be possible:

<?php

require 'vendor/autoload.php';

use Spatie\Period\Period;

$period = Period::make('2018-01-24', '2018-01-31');

foreach ($period as $date) {
    echo $date->format('Y-m-d').PHP_EOL;
}

That's where my question comes into play: I'm using iterator_count() to test if the Iterator iterates over the expected amount of dates, and noticed the length() method, of which I first thought that it does the same. However, length() always returns the amount of days in a Period, which was not was I expected.

Considering the following hour-based period (a Period with hour-precision) which I am also covering in the tests) I would have expected length() to return 24 instead of one.

<?php

require 'vendor/autoload.php';

use Spatie\Period\Period;
use Spatie\Period\Precision;

$period = Period::make('2018-01-24 00:00:00', '2018-01-24 23:59:59', Precision::HOUR);

echo iterator_count($period).' vs '.$period->length(); // -> 24 vs 1

That's why I didn't change the length() method to return the iterator count, because this would be a breaking change, but I would like to use the opportunity for this question:

Is Spatie's Period supposed to support Date periods exclusively, or is the vision to make it a replacement for/extension of the native DatePeriod similarly as Carbon and Chronos are for DateTime and DateTimeImmutable?

:octocat:

PS: I'm not sure why StyleCI wants me to do reorder the use statements as it suggests. I thought ordered imports where the way to go 😅

[brendt]

Hi Jérôme

Great PR and questions, there's a lot to think about!

First, let's address the length issue. It doesn't take the precision into account, and that's an oversight on my part. I think we should fix is, but that means a breaking change. I don't mind tagging a new major version though, so I'll make an issue: https://github.com/spatie/period/issues/18 . I hope you want to share your thoughts on it, because I still have some questions on the best approach.

Is Spatie's Period supposed to support Date periods exclusively

At this point, we have no ambition to replace Carbon or Chronos or any other date package. I know these packages also provide a period implementation, but they are missing some (or most) of the functionality this package aims to provide.

I'm using this package in combination with Carbon. I deliberately didn't make Period final, so that it can be extended. In the project I'm using it, I made my own implementation with extra Carbon-relted functionality, extended from the Period class.

I'll answer PR specific stuff in a review!

[jeromegamez]

At this point, we have no ambition to replace Carbon or Chronos or any other date package.

I realized today that my question about replacing Carbon/Chronos was not the brightest 😅. This package provides an approach to a specific challenge, I think I brought it up to check if you consider spatie/period feature complete :)

[jeromegamez]

Should I change the „length“ method to return iterator_count($this) in this PR (breaking change), or shall we wrap it up here as it is, and follow up based on the result of #18?

[brendt]

Let's keep length as is now, and introduce breaking changes with the Duration stuff.

I'll merge this PR maybe this weekend, or next week. (Attending PHP BNL today, so I don't know if I'll have time or not)

[brendt]

Here we go! Thanks for the contribution

[brendt]

Pressed the wrong button 🙈