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: nil check in logging conditional #1860

Merged
merged 3 commits into from
Sep 12, 2023
Merged

Conversation

cmaddox5
Copy link
Contributor

Asana task: ad-hoc

I forgot to check next_train_prediction for nil when logging. This is causing a good amount of errors overnight when all predictions are nil.

  • Tests added?

@cmaddox5 cmaddox5 requested review from a team and PaulJKim and removed request for a team September 11, 2023 14:41
@github-actions
Copy link

Coverage of commit 2a79cbb

Summary coverage rate:
  lines......: 40.8% (2292 of 5622 lines)
  functions..: 42.2% (1050 of 2490 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/candidate_generator/widgets/train_crowding.ex             |72.7%     33|55.6%     9|    -      0

Download coverage report

@@ -56,7 +56,7 @@ defmodule Screens.V2.CandidateGenerator.Widgets.TrainCrowding do
params |> Map.to_list() |> fetch_alerts_fn.() do
next_train_prediction = List.first(predictions)

if logging_options.is_real_screen do
if not is_nil(next_train_prediction) and logging_options.is_real_screen do
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I personally like if next_train_prediction && logging_options.is_real_screen do better for the brevity, but just personal preference. Not sure if using is_nil/1 is more idiomatic though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could've sworn I tried that earlier, but the problem is I was using and instead of &&... Thanks for the suggestion cause I definitely like this better.

@github-actions
Copy link

Coverage of commit 39559b2

Summary coverage rate:
  lines......: 40.8% (2292 of 5622 lines)
  functions..: 42.2% (1050 of 2490 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/candidate_generator/widgets/train_crowding.ex             |72.7%     33|55.6%     9|    -      0

Download coverage report

@github-actions
Copy link

Coverage of commit 59ce3dc

Summary coverage rate:
  lines......: 40.8% (2292 of 5622 lines)
  functions..: 42.2% (1050 of 2490 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/candidate_generator/widgets/train_crowding.ex             |72.7%     33|55.6%     9|    -      0

Download coverage report

@cmaddox5 cmaddox5 merged commit ecb878a into master Sep 12, 2023
2 checks passed
@cmaddox5 cmaddox5 deleted the cm/triptych-logging-error branch September 12, 2023 12:33
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