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 on_break? schedule method #72

Merged
merged 3 commits into from
Jun 13, 2016
Merged

Add on_break? schedule method #72

merged 3 commits into from
Jun 13, 2016

Conversation

craiglittle
Copy link
Collaborator

Similar to on_holiday? with holidays, this method can be used to determine if a provided time occurs during a break.

@zendesk/darko

end
end

context 'when the time is not on a break' do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This is fine, but I prefer when the time is not during a break for readability purposes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, on a does match up with the method name (on_break?) better, but it is also nice to provide richer/different descriptions in the docstrings to flesh out the intention. Will change!

@alex-stone
Copy link

👍, with or without suggested spec wording changes.

@craiglittle
Copy link
Collaborator Author

@alex-stone 💇

Intervals and holidays already excluded their endpoint when determining
if a time was within them. This makes it so time segments behave in the
same way. It also refactors `Holiday` a bit to delegate to its
underlying time segment for the operation.
Similar to `on_holiday?` with holidays, this method can be used to
determine if a provided time occurs during a break.
This wording is a bit richer and provides more context with respect to
the `on_*?` methods.
@alex-stone
Copy link

👍

@craiglittle craiglittle merged commit f23c43b into master Jun 13, 2016
@craiglittle craiglittle deleted the craig/on-break branch June 13, 2016 23:11
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