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

Parse signs config and store info in state to be used in group filter #350

Merged
merged 2 commits into from
May 7, 2024

Conversation

bfauble
Copy link
Contributor

@bfauble bfauble commented May 2, 2024

Summary of changes

Asana Ticket: Parse config and store info in state

  • Moves state logic into correct namespace
  • Parser for parsing signs config feed
  • Group filter uses data from state
  • TODO: Supervisor to do the actual fetching of feed to feed into state

@bfauble bfauble requested review from a team and nlwstein and removed request for a team May 2, 2024 22:15
Copy link

github-actions bot commented May 2, 2024

Coverage of commit cbad0d2

Summary coverage rate:
  lines......: 92.8% (1453 of 1565 lines)
  functions..: 80.9% (474 of 586 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/filter/suppress/stop_prediction_status.ex         | 100%     19| 100%     6|    -      0
  lib/concentrate/group_filter/suppress_stop_time_update.ex         |86.7%     15|75.0%     4|    -      0
  lib/concentrate/parser/signs_config.ex                            | 100%      6| 100%     2|    -      0

Download coverage report

@nlwstein
Copy link
Contributor

nlwstein commented May 3, 2024

This might be better-suited to a quick discussion / call, but could we deploy to a dev environment and validate this behavior manually? There's a lot going on here logic-wise, and I think if we both smoke tested it in dev that would be confidence-inducing.

The code itself looks sound syntactically 🙂

@bfauble
Copy link
Contributor Author

bfauble commented May 7, 2024

This might be better-suited to a quick discussion / call, but could we deploy to a dev environment and validate this behavior manually? There's a lot going on here logic-wise, and I think if we both smoke tested it in dev that would be confidence-inducing.

The code itself looks sound syntactically 🙂

At the moment at least, this isn't actually hooked up to run in dev/prod.

https://github.com/mbta/concentrate/blob/master/config/config.exs#L78

The next PR will include both uncommenting/enabling all this code as well as using 'live' data from the signs config to better test against. It'd be possible to add some temp commits here to fake using live data, but it would be based on assumed data format vs the actual feeds.

Copy link
Contributor

@nlwstein nlwstein left a comment

Choose a reason for hiding this comment

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

OK - that makes sense - I figure we can review this more conclusively once it's enabled in dev @bfauble. The code otherwise looks syntactically good so I think this is good to merge.

direction_id: direction_id,
stop_id: stop_id
} ->
Logger.info(fn ->
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: does this need to be an anonymous function? I think you can just pass an interpolated string directly into Logger.info.

Copy link

github-actions bot commented May 7, 2024

Coverage of commit 457414b

Summary coverage rate:
  lines......: 92.8% (1453 of 1565 lines)
  functions..: 80.9% (474 of 586 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/filter/suppress/stop_prediction_status.ex         | 100%     19| 100%     6|    -      0
  lib/concentrate/group_filter/suppress_stop_time_update.ex         |86.7%     15|75.0%     4|    -      0
  lib/concentrate/parser/signs_config.ex                            | 100%      6| 100%     2|    -      0

Download coverage report

@bfauble bfauble merged commit cde9d25 into master May 7, 2024
3 checks passed
@bfauble bfauble deleted the bf-signs-config-state branch May 7, 2024 19:53
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