-
Notifications
You must be signed in to change notification settings - Fork 3
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
Use local time fields #76
Conversation
My understanding of the new local time fields is that they will remain consistent during a service day, even if that service day includes a time change. Therefore, we can switch to using the local time fields, and discard our logic which we had constructed to deal with the feed error. Closes #9. Correct me if I'm wrong.
@dfaulken commented:
That is also my understanding. In the email from Avail last July:
Based on that, the LocalTime fields will not "spring forward" from 1:59:59 to 3:00:00 (or "fall back" from 1:59:59 to 1:00:00 in November). It's worth noting that at 1:55, a bus with an EDT of 2:05 (service date time) would technically be leaving in 10 min at 3:05 am in the spring (and in 10 min at 1:05 am in the fall), but I think it's fine to display the time as 2:05 am. Whichever time we display could end up confusing people, but hopefully the 10 min part of the departure board will make it clear. Side note: I just realized that this won't be an issue until November anyway due service ending early for Spring Break (the March DST switch didn't happen during Spring Break in 2014 and 2015, but it did in 2016 and 2017, and will in 2018 and 2019). |
So then this PR goes too far. When the current local time switches for instance from 2:00 am to 1:00 am, then a bus scheduled for 2:05 will show an absolute time of 2:05 am, but a relative time of "1 hr, 5 min", despite the fact that it arrives in 5 minutes. So we still need to maintain our offset logic, but on the other end of the time display. We need to remember what the offset was at the start of the day for the purposes of showing relative times, whereas previously we did it for the purposes of showing absolute times. Correct? |
I don't have a strong opinion about which time-of-day we show (if anyone else does, please chime in), but the minutes away should be accurate. |
Thinking about it a bit more, it might make more sense to show the times as 2:05 in my example (simply because that would correspond to the scheduled time). |
Okay. @werebus, does my analysis of what needs to be done here sound correct to you? |
Yes, we still need the offset. Although, alternatively, it might be better to know what UTC offset to use for the service day instead. Moment still knows something about DST even with "local" times: moment('2018-03-11T01:30:00').format('h:mm a')
//=> "1:30 am"
moment('2018-03-11T02:30:00').format('h:mm a')
//=> "3:30 am"
moment('2018-03-11T03:30:00').format('h:mm a')
//=> "3:30 am" That is, when we parse "Local" times for math purposes from Avail they should be moment('2018-03-11T01:30:00-0500').format('h:mm a')
//=> "1:30 am"
moment('2018-03-11T02:30:00-0500').format('h:mm a')
//=> "3:30 am"
moment('2018-03-11T03:30:00-0500').format('h:mm a')
//=> "4:30 am" But, as is evident from my second example, we should (I guess) display them unmodified. Those times are on the printed timetables as "1.30", "2:30", and "3:30" respectively. |
This PR is from more than 3 years ago and isn't correct yet. I think I'm going to close it. If someone wants to work on #9 still, it would probably be better to start from scratch. |
My understanding of the new local time fields is that they will remain consistent during a service day, even if that service day includes a time change.
Therefore, we can switch to using the local time fields, and discard our logic which we had constructed to deal with the feed error. Closes #9.
Correct me if I'm wrong.