New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Length with precision #18

Open
brendt opened this Issue Jan 25, 2019 · 8 comments

Comments

Projects
None yet
2 participants
@brendt
Copy link
Member

brendt commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Collaborator

jeromegamez commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Member Author

brendt commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Collaborator

jeromegamez commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Member Author

brendt commented Jan 26, 2019

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

@brendt

This comment has been minimized.

Copy link
Member Author

brendt commented Jan 29, 2019

@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

This comment has been minimized.

Copy link
Collaborator

jeromegamez commented Jan 29, 2019

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

@jeromegamez

This comment has been minimized.

Copy link
Collaborator

jeromegamez commented Jan 30, 2019

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-31). 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

This comment has been minimized.

Copy link
Member Author

brendt commented Jan 30, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment