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

chore: refactor function application over source_config values #798

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

sloanelybutsurely
Copy link
Contributor

@sloanelybutsurely sloanelybutsurely commented Jul 22, 2024

Summary of changes

Defines map_source_config/2, a function for applying a function to SourceConfig values (both single and top/bottom--tuple).

Uses the new map_source_config/2 function to calculate alert_status, first_scheduled_departures, and service_end_statuses_per_source.

Reviewer Checklist

  • Any new or changed functions have typespecs
  • Tests were added for any new functionality (don't just rely on Codecov)
  • This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes (compare on Splunk: staging vs. prod)

@sloanelybutsurely sloanelybutsurely force-pushed the sloane/support-split-alert-status branch from 836b0f0 to 26a4332 Compare July 22, 2024 15:02
@sloanelybutsurely sloanelybutsurely changed the title Push pmnroptouklr feat: add SourceConfig.map/2 and SourceConfig.simplify/1 Jul 22, 2024
@mbta mbta deleted a comment from github-actions bot Jul 22, 2024
@sloanelybutsurely sloanelybutsurely force-pushed the push-pmnroptouklr branch 2 times, most recently from 33c4f7c to 5cd10ec Compare July 22, 2024 18:22
@mbta mbta deleted a comment from github-actions bot Jul 22, 2024
@digitalcora
Copy link
Contributor

digitalcora commented Jul 22, 2024

Since these are both only used in one module, and simplify in particular only has a single use, extracting them into public functions on SourceConfig feels a bit premature — unless this intentionally doesn't refactor all the places this could be used? (you mentioned "throughout the application")

The improvement in readability for the calling code is nice, though. Maybe these could be private functions?

Edit: I'm just noticing this is a Draft PR I've nosed my way into. Please disregard if still in progress!

@sloanelybutsurely sloanelybutsurely force-pushed the sloane/support-split-alert-status branch from 90c4be5 to 80cdebc Compare July 23, 2024 13:44
@mbta mbta deleted a comment from github-actions bot Jul 24, 2024
@mbta mbta deleted a comment from github-actions bot Jul 24, 2024
@sloanelybutsurely
Copy link
Contributor Author

@digitalcora lol hey cora! yeah this is just something i pushed up as an example (referenced here #797 (comment))

@sloanelybutsurely sloanelybutsurely force-pushed the sloane/support-split-alert-status branch from 3153b1d to c595cc0 Compare July 24, 2024 18:24
@sloanelybutsurely sloanelybutsurely force-pushed the push-pmnroptouklr branch 2 times, most recently from 200310d to f24bf44 Compare July 24, 2024 18:41
@sloanelybutsurely sloanelybutsurely changed the title feat: add SourceConfig.map/2 and SourceConfig.simplify/1 chore: refactor function application over source_config values Jul 24, 2024
@sloanelybutsurely sloanelybutsurely marked this pull request as ready for review July 24, 2024 18:44
@sloanelybutsurely sloanelybutsurely requested a review from a team as a code owner July 24, 2024 18:44
@sloanelybutsurely
Copy link
Contributor Author

@digitalcora I've incorporated your feedback to make this a more reasonable refactoring change. I've changed this to only define map_source_config/2 as a private function in Signs.Realtime.

@mbta mbta deleted a comment from github-actions bot Jul 24, 2024
@mbta mbta deleted a comment from github-actions bot Jul 24, 2024
@mbta mbta deleted a comment from github-actions bot Jul 24, 2024
Base automatically changed from sloane/support-split-alert-status to main July 24, 2024 19:03
Defines `map_source_config/2`, a function for applying a function to
SourceConfig values (both single and top/bottom--tuple).

Uses the new `map_source_config/2` function to calculate `alert_status`,
`first_scheduled_departures`, and `service_end_statuses_per_source`.
Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

@sloanelybutsurely sloanelybutsurely merged commit 6633355 into main Jul 25, 2024
2 checks passed
@sloanelybutsurely sloanelybutsurely deleted the push-pmnroptouklr branch July 25, 2024 15:37
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