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

A period must contain at least its starting datepoint #31

Closed
KrisLamote opened this Issue Feb 9, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@KrisLamote

KrisLamote commented Feb 9, 2016

Hi - first of all thanks for the small but sweet package, will be useful for overlapping periods validation.

While going through the code I noticed that the constructor is accepting the same $startDate and $endDate, with a notable exception message 'the ending endpoint must be greater or equal to the starting endpoint'.

On the other hand the contains method expects the input $datetime to be: stricktly smaller than the endDate. This suggests to me that the ending endPoint is considered to be outside the period. This last bit doesn't seem to be in line with the check in the constructor?

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Feb 9, 2016

Member

This suggests to me that the ending endPoint is considered to be outside the period.

This is as expected and as described in the documentation.

While going through the code I noticed that the constructor is accepting the same $startDate and $endDate, with a notable exception message 'the ending endpoint must be greater or equal to the starting endpoint'.

Because you need to be able to represent 0 duration Period. I don't see the inconsistency maybe you coud give me a concrete example of inconsistency ?

Member

nyamsprod commented Feb 9, 2016

This suggests to me that the ending endPoint is considered to be outside the period.

This is as expected and as described in the documentation.

While going through the code I noticed that the constructor is accepting the same $startDate and $endDate, with a notable exception message 'the ending endpoint must be greater or equal to the starting endpoint'.

Because you need to be able to represent 0 duration Period. I don't see the inconsistency maybe you coud give me a concrete example of inconsistency ?

@KrisLamote

This comment has been minimized.

Show comment
Hide comment
@KrisLamote

KrisLamote Feb 9, 2016

Agreed, re-reading the documentation - that's clear, please close this

Conversely, the ending datepoint is excluded from the specified period.

I would consider the starting endPoint to always be inside the Period. If the starting endPoint and the ending endPoint are the same, then the starting endPoint is also outside the Period -- that last bit sounds a bit strange to me. Specifically if you rephrase it as follows: sometimes the starting endPoint is inside the Period, but usually (when the duration > 0) it's inside :)

I also understand that diff, abuts and intersect highly depend on this concept - so I will also adopt your point of view.

cheers & thanks

KrisLamote commented Feb 9, 2016

Agreed, re-reading the documentation - that's clear, please close this

Conversely, the ending datepoint is excluded from the specified period.

I would consider the starting endPoint to always be inside the Period. If the starting endPoint and the ending endPoint are the same, then the starting endPoint is also outside the Period -- that last bit sounds a bit strange to me. Specifically if you rephrase it as follows: sometimes the starting endPoint is inside the Period, but usually (when the duration > 0) it's inside :)

I also understand that diff, abuts and intersect highly depend on this concept - so I will also adopt your point of view.

cheers & thanks

@nyamsprod nyamsprod closed this Feb 9, 2016

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Feb 10, 2016

Member

After thinking about it, I think you are indeed correct this is a bug, even a 0 duration Time range has at least a starting datepoint 👍 . I'll release a minor fix because the change does not affect other comparison methods.

Member

nyamsprod commented Feb 10, 2016

After thinking about it, I think you are indeed correct this is a bug, even a 0 duration Time range has at least a starting datepoint 👍 . I'll release a minor fix because the change does not affect other comparison methods.

@nyamsprod nyamsprod reopened this Feb 10, 2016

@nyamsprod nyamsprod added the bug label Feb 10, 2016

nyamsprod added a commit that referenced this issue Feb 10, 2016

bug fix Library
- A Period with a empty DateInterval must at least contains its starting datepoint #31
- microseconds should be taken into account when converting DateTimeInterface objects

@nyamsprod nyamsprod changed the title from Inconsistency between constructor and contains to A period must contain at least its starting datepoint Feb 10, 2016

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Feb 12, 2016

Member

the fix has been released

Member

nyamsprod commented Feb 12, 2016

the fix has been released

@nyamsprod nyamsprod closed this Feb 12, 2016

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