Skip to content
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

Add shift.overlaps? and shift.contains? #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

righi
Copy link
Contributor

@righi righi commented Sep 9, 2014

No description provided.

@jackc
Copy link
Owner

jackc commented Sep 10, 2014

These look useful, but I've got a couple questions.

include? already tests if a shift contains a TimeOfDay. Should the functionality of contains? be folded into include?. Then there would just be one method for Shift to test the inclusion of something, regardless of whether that something was another Shift or a TimeOfDay (could also extend this to test a Time).

include? treats the shift as an inclusive range. This can make sense for testing inclusion of a TimeOfDay. For example, it is reasonable to say that a Shift from 8am to 4pm includes the TimeOfDay 4pm. But it feels counter intuitive to say that a Shift of 8am to 4pm overlaps 4pm to 11pm. Typically ranges for scheduling purposes are [) (inclusive start / exclusive end). It makes me wonder if I made a mistake with treating Shift as inclusive on both ends. I'm not sure of a good solution to this. We could:

  • overlaps? continues to treat Shift as inclusive on both ends -- prevents shifts from bumping up against each other without overlap.
  • overlaps? treats Shift as a [) range -- include? and overlaps? would have different behavior.
  • include? changes to treat Shift as [) range -- breaking change (if anyone is relying on that)

Any ideas on a good resolution?

@jackc
Copy link
Owner

jackc commented Jan 16, 2015

I've decided to release v2 of Tod with some small breaking changes. v2 supports Shifts with exclusive ends, just like Ruby ranges. I've merged this into the v2 branch. I tweaked overlaps? to work with both inclusive and exclusive ends.

Would you take a look at the v2 branch? I'm planning on releasing it in the next week or so depending on feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants