Let length() use the Period's precision

[jeromegamez]

length() now uses the Period's precision.

:octocat:

---

Original PR text for Reference:

This adds a new method amountOfIncludedDates() and deprecates the method length() which currently doesn't account for a given precision.

I'm not a 100% sure about the name of the method, but length() seemed too ambiguous to me, so

"How long is this period?" -> "It's 24 long"

would become

"How many dates are included in my Period, based on the precision?" -> "There are 24"

Just deprecating the length() method would allow us to release a non-breaking 1.2 instead of a breaking 2.x. - when developers update the library through composer update and haven't disabled deprecation warnings in their dev/test environments, they will receive the information about the deprecated method and can change their code accordingly to be able to upgrade to the next major release with ease 🎉.

If they have enabled deprecation warnings in live environments… well, then this deprecation warning will teach them not to do that 😅

:octocat:

[brendt]

I think length still has its place though. It's true that it doesn't give any context, but in many projects, you'll only ever use one kind of precision for all periods. In this case length is a handy shortcut for saying "give me the length of this period for its current duration".

What if we added a ->duration() method, which return an object of Duration. This Duration object not only contains the length, but also the precision. I'd like to keep length() on the Period class, but just as a shortcut:

public function length(): int
{
    return $this->duration->length();
}

If we have a Duration class, we could also do some of the comparisons in that class:

// class Period

public function greaterThan(Period $period): bool
{
    return $this->duration->greaterThan($period->duration);
}

Same way you proposed in #18. What do you think?

[jeromegamez]

The idea to bind the Period into a Duration object didn't occur to me, probably because I saw it as a pure value object so far, but it's certainly worth a try.

On the other hand, wouldn't we "just" create a new object to outsource some methods that partially already are in the Period? There is already the equals() method in the Period class, and I could imagine that there could be confusion and incoming support requests on edge cases like

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

$a->length() === $b->length(); // true
$a->duration()->equals($b->duration()); // false
assert($a->equals($b)); // Exception

I realize these are edge cases, most people will use the package as intended and compare Periods with the same precision. I'm always concerned about edge cases (I'm old and have met too many of them 😂), so I'm looking forward for your thoughts on this!

In the meantime, I'll update the PR to revert most of the changes so that length() behaves again 😅

[jeromegamez]

PR updated (and the diff a tad smaller, which is nice ^^)

I'm thinking: If we code-freeze here, we could create a 2.0 release and add additional functionality in a 2.x release.

[brendt]

I think changing length to take the duration into account is already a breaking change. Merging this would mean tagging 2.0.

I'm perfectly fine with that, and I think it's a good time to think about what other stuff we can add in a 2.0 release. I'll make a separate issue for that.

So how about making a new 2.0 branch, and targeting this PR to that one? Than we can think about what things to add before releasing 2.0. What do you think?

Feel free to make the branch and merge this PR in it, it looks fine to me!

[brendt]

Also, I quickly wanted to respond to your concern about comparing lengths:

$a->length() === $b->length(); // true
$a->duration()->equals($b->duration()); // false
assert($a->equals($b)); // Exception

This package is profiled as "Periods with precision", of course you're able to compare two lengths manually, they are simple integers after all; but that's in many cases not the correct use.

I think we should clearly add a note about this to the README, but still allow length() to be exposed as is, because I can imagine most people would use this library with one type of precision for all periods anyways.

We'll discuss the duration stuff in #22