Off-by-one error in `get_as_minute_array`

[berkes]

Parsing a week and converting to a byte-array with get_as_minute_array results in 1441 minutes per day. A day only has 1440 minutes. I assume 00:00/24:00 are counted twice. Once as beginning minute and once as ending minute.

This can be reproduced with:

parsed  = OpeningHoursConverter::OpeningHoursParser.new('Mo-Su 10:00-18:00')
week = parsed.first.typical
week.get_as_minute_array.each do |day|
   puts day.count
end
#> 
1441
1441
1441
1441
1441
1441
1441

This is probably a bug. Especially since it is undefined what minute is added.

[Mziserman]

I think you are right, it is a bug. The problem is a lot of the lib logic uses this array, I'll have to look what a modification here implies.

I think the easy fix is https://github.com/Publidata/opening_hours_converter/blob/master/lib/opening_hours_converter/day.rb#L13 removing + 1 here

[berkes]

I'm not 100 sure yet.

Say you have opening-hours of: 00:01-00:02,23:57-23:58 Which is practically a weird case, and meaning that the place is open in the 2nd AND 1-but-last minute. In total for 2 minutens. We would then expect a minute-array for that day, of:

0,1,0,0...0,0,1,0

I think by removing the +1 we simply shift everything, so it now becomes 1,0,0,0...0,1,0,01 which is even more incorrect than having what we have now:

0,1,0,0...0,0,1,0,0.

I'll write a test with this usecase and see if I can a) fix this and b) reproduce it that way.

But first: Would you agree that the first minute of the day is 00:00-00:01, and the last minute of the day is 23:58-23:59? I'm not entirely sure if this is a cultural thing and that some countries might consider 00:00 to be part of the previous day? (And if so, how do they do the countdown on NYE?)

[Mziserman]

I agree that the first minute of the day should be 00:00-00:01.

OpeningHoursConverter::OpeningHoursParser.new.parse("2019 Jan 10 00:01-00:02,23:57-23:58").first.typical.get_as_minute_array.map.with_index do |value, index|
  index if value
end.compact
=> [1, 2, 1437, 1438]

So actually the current implementation returns

0,1,1,0...0,1,1,0,0

I remember now, the initial reason it was encoded like that (setting true for the entire end minute does not seem logical) was to handle point in time opening hours (Mo 10:00 vs Mo 10:00-10:01) which we do not anymore.

I agree that

00:01-00:02,23:57-23:58  => 0,1,0,0...0,0,1,0

would make more sens.

But this explains the 1441 length array since you have to have 24:00 to encode 23:59-00:00 with this format

[berkes]

I fixed this in both week.rb and day.rb and their relevant tests.

Weird is that the day.rb has no effect on the other tests. But the change to week.rb starts failing 8 unrelated tests.

The get_as_minute_array is used in some complex-loopy-loop code, that I simply don't understand. So I cannot fix it.

The code is found in commit:f25e1ff but as the commit message sugggests: it is not finished nor working.