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

fix: refactor of routes-serving-stop fetching #2120

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

digitalcora
Copy link
Contributor

@digitalcora digitalcora commented Aug 1, 2024

The version of this function prior to the refactor in 297c302 returned only a list of routes, returning an empty list if an error occurred. The moved/renamed version returns an :ok tuple or :error, consistent with other data fetching functions, but the call sites were not updated to reflect this (😓), and no tests failed because the mocks for the function still used the old return type (or, in the case of ElevatorClosures, the module lacked any tests at all).

This keeps the return type of the fetching function the same, so errors can be distinguished if needed, but changes the existing call sites to perform the "default to empty list" logic themselves, so there should be no change in behavior.

Deployed this branch to dev-green so we should see errors if there are still any, from the self-refresh runner.

The version of this function prior to the refactor in 297c302 returned
only a list of routes, returning an empty list if an error occurred.
The moved/renamed version returns an `:ok` tuple or `:error`, consistent
with other data fetching functions, but the call sites were not updated
to reflect this, and no tests failed because the mocks for the function
still used the old return type (or, in the case of `ElevatorClosures`,
the module lacked any tests at all).

This keeps the return type of the fetching function the same, so errors
can be distinguished if needed, but changes the existing call sites to
perform the "default to empty list" logic themselves, so there should be
no change in behavior.
@digitalcora digitalcora marked this pull request as ready for review August 1, 2024 15:23
@digitalcora digitalcora requested a review from a team as a code owner August 1, 2024 15:23
Copy link

github-actions bot commented Aug 1, 2024

Coverage of commit 08cc124

Summary coverage rate:
  lines......: 45.2% (2954 of 6538 lines)
  functions..: 40.3% (1053 of 2610 functions)
  branches...: no data found

Files changed coverage rate:
                                                                          |Lines       |Functions  |Branches    
  Filename                                                                |Rate     Num|Rate    Num|Rate     Num
  ==============================================================================================================
  lib/screens/routes/route.ex                                             |60.0%     70|57.1%    21|    -      0
  lib/screens/v2/candidate_generator/dup/departures.ex                    |85.8%    148|75.0%    24|    -      0
  lib/screens/v2/candidate_generator/widgets/elevator_closures.ex         | 0.0%     25| 0.0%     9|    -      0

Download coverage report

Comment on lines +69 to +74
defp routes_serving_stop(stop_id) do
case Route.serving_stop(stop_id) do
{:ok, routes} -> routes
:error -> []
end
end
Copy link
Contributor

@sloanelybutsurely sloanelybutsurely Aug 1, 2024

Choose a reason for hiding this comment

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

off-topic: maybe someday we'll have something like fromMaybe from Data.Maybe... :: sigh ::

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I interest you in some Gleam? 😺

@digitalcora digitalcora merged commit f6a1805 into main Aug 1, 2024
15 checks passed
@digitalcora digitalcora deleted the cfg-fix-routes-serving-stop branch August 1, 2024 17:57
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