search

spatie / period

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

Add a Period->coversDate method #8

Closed IwfY closed 5 years ago

IwfY commented 5 years ago

I have use cases for this library but what prevents me from using it is a convenient Period->coversDate method that receives a DateTime object and two mode information for whether the start/end of the period are included or excluded. The mode may also be be combined to a single parameter.

Would this be possible and within the scope of this library?

brendt commented 5 years ago

@IwfY definitely! Are you up for sending a PR with tests?

IwfY commented 5 years ago

@brendt Unfortunately not at the moment. I'm sorry.

brendt commented 5 years ago

No problem, I'll add it.

Just to be clear about the mode thing: you want to be able to include or exclude the start and end date of the period, right? Can you give an example of when you'd want to exclude the boundaries?

IwfY commented 5 years ago

I work with the convention to always include the start and exclude the end. But I thought it was more in line with the existing methods like startAfter and startAfterOrAt.

brendt commented 5 years ago

Are there any known benefits to excluding the end date?

IwfY commented 5 years ago

I need time sensible periods. I know that this package is/was primary based on days. That is why the scope is important to decide.

The advantage for me is that when I'm working with sequential periods I can set the start of the second period and the end of the first to the same moment in time and don't have to subtract an arbitrary fraction of seconds from this moment to have a non-overlapping end for the first period.

brendt commented 5 years ago

I see your point 🤔

I find it difficult to decide what's better. On the one hand excluding the end date is convenient, but it also adds some cognitive load. The developer has to keep in mind this unwritten rule.

I'm going to open a separate issue to further discuss this. For now Period::contains has been added and tagged as 0.3.0: https://github.com/spatie/period/releases/tag/0.3.0

brendt commented 5 years ago

For reference, we'll discuss the boundaries more in depth in this issue: https://github.com/spatie/period/issues/9

IwfY commented 5 years ago

@brendt I think something went wrong with this commit!? I don't see the contains method in this repo.

brendt commented 5 years ago

Oh that's embarrassing 😅 it's tagged correctly now. I also tagged 0.3.1 to avoid packagist caching issues 🙃