Gracefully handle no departures for Line Map

[sloanelybutsurely]

Asana task: [Screens 🐞] Gracefully handle no departures for Line Map

Description

Handles the case when there fewer than two departures with predictions but also no departures without a prediction.

• Previously this code would pass nil to Screens.V2.Departure.time/1 which raised an exception

Other changes

• Refactors counting of departures with predictions to use a single Enum.count/2
  • I'm happy to remove this if you all feel like it is out-of-place in this changeset

Known issues / thoughts

This doesn't eliminate all possible errors related to the nilability of departures and the arrival and departure fields within schedules and predictions but is instead focused on the specific error reported to Sentry. A refactor to account for that class of errors would have a much larger surface area. Happy to address this if needed!

[sloanelybutsurely]

note: As called out in the PR description this isn't really related to this bug. I'm happy to remove this if you'd like.

[github-actions]

Coverage of commit c525734

Summary coverage rate:
  lines......: 44.5% (2845 of 6394 lines)
  functions..: 42.0% (1019 of 2428 functions)
  branches...: no data found

Files changed coverage rate:
                                                                          |Lines       |Functions  |Branches    
  Filename                                                                |Rate     Num|Rate    Num|Rate     Num
  ==============================================================================================================
  lib/screens/v2/widget_instance/line_map.ex                              |73.0%     74|82.6%    23|    -      0

Download coverage report

[digitalcora]

I'm not a huge fan of nesting if expressions. This function could further refactored to use a with expression which would also return nil in cases where there are no passed stops, DateTime.shift_zone/2 fails, or Timex.format/2 fails. However, I wasn't sure if those possible runtime errors were desired so I left them in.

with expressions can include = assignments, which behave like "normal" assignments and crash if the pattern doesn't match. So if it improves clarity, it should be possible to reformat it as a with and not change the existing behavior. (I'd say crashes are desirable if they would always indicate a "programmer error", so they will cause a test failure or a Sentry report and we can fix them — and I think at least some of these fall into that category.)

[github-actions]

Coverage of commit 44487c8

Summary coverage rate:
  lines......: 44.5% (2845 of 6394 lines)
  functions..: 42.0% (1019 of 2428 functions)
  branches...: no data found

Files changed coverage rate:
                                                                          |Lines       |Functions  |Branches    
  Filename                                                                |Rate     Num|Rate    Num|Rate     Num
  ==============================================================================================================
  lib/screens/v2/widget_instance/line_map.ex                              |73.0%     74|82.6%    23|    -      0

Download coverage report

[sloanelybutsurely]

@digitalcora hmmm there were more = than there were <- but i realized that the two if expressions could be combined so i went with that: 44487c8

[cmaddox5]

This looks great. Definitely like the most recent commit over the initial solution. I'm fighting myself on whether the other errors could be in scope just because the screen can still be useful if we don't have this piece. If another error happened, it would be good to log it but should let the screen render what it can. I agree with @digitalcora on the definition of a good crash, but don't think the date functions would error because of anything but a data issue. Would love to hear other thoughts though.

[digitalcora]

Looking at the trade-off of the added complexity to log an error and continue anyway in this specific case, versus how likely the errors are to occur... I'd say it's questionably worth it. It's not like we do this in any disciplined fashion everywhere we use e.g. Departure.time currently, and if we suspected this was a data issue worth guarding against, we should build it into the return value of that function. (Given the arguments are in fact valid datetimes and zone names / format strings, I don't think DateTime.shift_zone or Timex.format would ever error.)

[sloanelybutsurely]

Given the arguments are in fact valid datetimes and zone names / format strings, I don't think DateTime.shift_zone or Timex.format would ever error.

@digitalcora FWIW, based on the typespecs of the Departure, Schedule, and Prediction structs, it is possible for Departure.time/1 to return nil.

Example

iex(3)> departure = %Screens.V2.Departure{prediction: %Screens.Predictions.Prediction{}, schedule: %Screens.Schedules.Schedule{}}
%Screens.V2.Departure{
  prediction: %Screens.Predictions.Prediction{
    id: nil,
    trip: nil,
    stop: nil,
    route: nil,
    vehicle: nil,
    alerts: [],
    arrival_time: nil,
    departure_time: nil,
    stop_headsign: nil,
    track_number: nil
  },
  schedule: %Screens.Schedules.Schedule{
    id: nil,
    trip: nil,
    stop: nil,
    route: nil,
    trip_id: nil,
    arrival_time: nil,
    departure_time: nil,
    stop_headsign: nil,
    track_number: nil,
    direction_id: nil
  }
}
iex(4)> Screens.V2.Departure.time(departure)
nil

However, I feel like the juice probably isn't worth the squeeze if we aren't seeing issues like this more frequently.