WorkingHours.return_to_working_time() doesn't pass .in_working_hours?

[rafalyesware]

When one returns a time to working time, it would seem sensible that the time actually passes the .in_working_hours? test in all cases.

However, if the time passed to return_to_working_time was outside working hours to start with, then WorkingHours.return_to_working_time will return a time that does not pass the .in_working_hours? predicate (because it returns the exact closing time).

See for example:

2.2.2 :006 > Time.now.in_working_hours?
=> false
2.2.2 :007 > WorkingHours.return_to_working_time(Time.now).in_working_hours? 
=> false
2.2.2 :008 > (WorkingHours.return_to_working_time(Time.now) - 1.second).in_working_hours?
=> true

[rafalyesware]

Maybe this needs a hack like end_of_day has, and shaves a nanosecond or so off the previous business day's closing time.

[jarthod]

Interesting, but tricky to do well, as it must not interfere with, for example:
Time.now - 8.working.hours — when outside a 8 hours working period, this should take us to the exact beginning of the period if I'm not mistaken.

It also must not break the iteration pattern present in next_working_time, as it iterates between periods by calling advance_to_closing_time to go at the end and then advance_to_working_time to find the next. If in_working_hours? returns true we'll call advance_to_closing_time in more cases an need to make sure it doesn't tamper the final time.

The behavior you want looks logical but is not really IMO, as ranges in computer science are often defined by one inclusive end and one exclusive for a reason: it's more precise.

Imagine someone who doesn't care about seconds or minutes and just want to know if a given hour is in his business time, returning true for the end of the range would be a 1 hour long mistake.

So I don't think this is worth the trouble implementing for the general case (and I'm pretty sure we'll get people asking for the opposite then)

What I can offer you is to add an option to the in_working_hours? method to be inclusive on the end range, this is easy to do and won't break anything.

[rafalyesware]

I realize it's tricky, which is why I filed a bug report and not a PR ;)

I agree that your use case of 'is 17:00 a working hour' is reasonable, but I also think if I'm asking to be returned to the last working time, then I should be given a time that's within working hours, not one outside... otherwise, you're breaking the contract of return_to_working_time.

Whether the range is an open or closed range is irrelevant... that's implementation detail to me as a user of the module. All I want to make sure is if I have asked to be returned to working hours, I am indeed within those hours.

[jarthod]

I see, then maybe we should rename this method, and have another one with this name returning an offseted time to be inside the period ?

[rafalyesware]

That seems like a reasonable way forward w/out messing up what the internals expect when walking the time list.

[jarthod]

Ok, can you do that @Intrepidd ?

[Intrepidd]

Yup whenever I got a chance

[Intrepidd]

Any idea on the naming ?

return_to_exclusive_working_time & return_to_inclusive_working_time ?

I'll keep return_to_working_time unchanged for now and just add a deprecation warning so we can remove it altogether in a next major release, WDYT ?

[jarthod]

My suggestion is keeping return_to_working_time and add return_after_working_time.
The after version being our current implementation.

[Intrepidd]

Why not but in this case we must bump the major version number as this is a
breaking change
On Sat, 26 Dec 2015 at 10:45, Adrien Jarthon [email protected]
wrote:

My suggestion is keeping return_to_working_time and add
return_after_working_time.
The after version being our current implementation.

—
Reply to this email directly or view it on GitHub
#28 (comment)
.

[jarthod]

Or maybe return_inside_working_time and return_outside_working_time ? the problem with all these is that if you are inside and call return_outside_working_time or return_after_working_time, you'll stay inside. So it's a bit misleading IMO.
We don't have to release a major for that IMO, It's a small change that's unlikely to break existing code.
Just a minor with a note in the changelog