Length with precision

[brendt]

The length method always returns the length in days, instead of the precision of the period. An oversight by me. We're going to fix it and release a new major version.

My suggestion is the following:

• Change length to return the length with the precision of that period,
• Add helper methods lengthInDays, lengthInYears,...

One more thing I'm wondering about: should length return a simple integer, or should it return some kind of PeriodLength object, which also contains the precision?

[jeromegamez]

I'm definitely in favor of changing length() as you described.

Concerning the helper methods... I thought about that as well and was hoping to shamelessly plug my own package for inclusion 😅. If I'm able to convince you to include gamez/duration, we could add a method getDuration() to Period and perhaps get(Total)Duration() to PeriodCollection so that we could do even more comparisons/operations, e.g.

assert($period->duration()->isLargerThan($otherPeriod->duration());
assert($period->duration()->equals($otherPeriod->duration());
// ...

length() could then perhaps receive an optional precision mask parameter - the default would then be the precision that the Period was created with. This would avoid a possible cluttering of multiple lengthIn* methods.

[brendt]

Hi Jérôme

That is a nice package, and I like the Duration idea! I'm a little hesitant to include a dependency in this package though 🤔

Would you consider merging the two packages into one? Please don't feel pressured to do so, I'm just thinking aloud.

[jeromegamez]

I totally understand you, it‘s feels better having no external dependencies.

The thing is (speaking of dependencies): a period depends on two points in time (and has a duration), but a Duration depends just on time. (It‘s something that I realized while working on a tool that syncs working times from a duration based time tracking tool to a period based one, „I worked for a certain duration“ vs „I worked from x to y“, meh)

I don‘t feel pressured (rather honored, and at the same time „noooo 😭) :)), but I think the duration package is good as a separate package, in case somebody only cares about durations, but not about periods.

That being said, that doesn‘t mean we can‘t copy the duration class into the Period package (unless I change the code license after writing this reply 😈). I would of course prefer not to (vanity 😅), but if you do, I can make a pull request introducing a Spatie\Period\Duration - it would probably be a little simpler and tied to/focused on the Period (no complicated constructor, for example), but could still turn out nice.

What do you think?

[brendt]

Let me think about it today, and I'll come back at it!

[brendt]

@jeromegamez I gave this some thought.

I don't think that adding an extra dependency to this package is an option, I'm sorry for that.

Though I do think that adding duration functionality would be a good feature. The way I see it, there are two options:

• You send a PR with the functionality, based on the duration package but tailored for this one.
• I can add the duration functionality myself, it'll probably be similar to yours.

I've got another question for you, but I'll send it via mail.

[jeromegamez]

I'm happy to create a PR adding the duration functionality, I'll get to it this week, probably rather sooner than later ^^

[jeromegamez]

Good call for not including the duration package ^^ - I tried to implement it yesterday in the Period package and ran into deal breakers…

A duration (and for that matter a DateInterval) is a relative value, while a Period is based on absolute days. That causes problems when comparing periods like (2018-03-01, 2018-03-31) and (2018-04-01, 2018-04-30). When representing both periods as Duration/DateInterval, the value would be P1M (= 1 month). So while these two are equal in terms of being a full month, they are not in terms of the number of included days. Similar problem with weeks/days in a week.

Long story short: I don't think anymore that a Duration would be a good fit for the Period, and we should probably go with adding the comparison methods to the period directly.

It's a bummer 😅, but if you agree, I would start working towards that direction instead.

[brendt]

Ah yes, very good point! Let's see where implementing it in the Period class leads us! The class itself is quite large already, and I was thinking about adding some kind of PeriodComparator class. The way I imagined it, is that a Period would still have the same public API, but use PeriodComparator internally. But let's first focus on adding the functionality to the Period class for now, and look at refactoring it in a later stage.

[jeromegamez]

This is now unresolved again, but we‘ll figure something out 💪

[wh120]

Period::make('2018-01-01 08:00:00', '2018-01-01 10:00:00', Precision::MINUTE)->length() // =1 😢😭😢 😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢😭😢

[brendt]

@wael120120 as a quick fix you could add your own lengthInMinutes helper by extending the Period class.

[gshzn]

I was just looking to get started on first open source contributions, and noticed this issue still being open.

Isn't this something that's fixed on the 2.0 branch using the precisionMask attribute on the Period class?

If not, please let me know. I'd be happy to take a further look at this.

[brendt]

I should look into it again, the truth is that we haven't had much need to finish v2 since we've never had a usecase besides days ourselves. Hence there's little need for us to push v2. But I hope to look into it soon-ish.

[ameenross]

If I may chime in with my humble opinion... PHP already has a DateInterval class, so it would be really cheap to add a function that returns one. We're talking a one-liner... return $this->start->diff($this->end);. In fact, the length function already makes use of that. I'm personally not a big fan of DateInterval, but it probably suits the needs of plenty of other users. Also, adding a separate function to do that means it's non-breaking.

Another common thing to do is to get the actual elapsed time between the dates. This makes use of Unix timestamps to ignore DST issues, and even timezone differences. $this->end->getTimestamp() - $this->start->getTimestamp(). Again, this could be a separate function to prevent BC breaks.

[ameenross]

Someone interested in using duration-php in tandem with period can just up-convert the DateInterval: Duration::make($period->getInterval());

[brendt]

will be fixed in v2