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

announce four-car trains #883

Merged
merged 2 commits into from
Feb 13, 2025
Merged

announce four-car trains #883

merged 2 commits into from
Feb 13, 2025

Conversation

panentheos
Copy link
Collaborator

Summary of changes

Asana Ticket: Approaching announcement for 4-car trains
Asana Ticket: New PA/ESS state when next train is 4-car

This modifies the approaching announcement and prediction rendering to include special messages for 4-car trains. See tickets for full details.

@panentheos panentheos requested a review from a team as a code owner February 11, 2025 19:40

[the_next_or_following] ++ train ++ prefix ++ status ++ suffix ++ followup
[the_next_or_following] ++ train ++ prefix ++ status ++ suffix ++ followup ++ four_cars
Copy link
Contributor

Choose a reason for hiding this comment

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

followup here is possibly something like "we will announce the platform for boarding soon", so does it make sense to then say "move to the front of the platform to board", when riders don't yet know which platform they should move to the front of?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fortunately (?) that followup message will only trigger on the JFK mezzanine, where we don't do passive readouts for four-car predictions. So as of now, this won't be an issue, but this is a good callout because there's a lot of stuff piling up here, and we could easily back ourselves into some nonsensical messaging if we're not careful about it.

Copy link
Contributor

@digitalcora digitalcora Feb 11, 2025

Choose a reason for hiding this comment

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

that followup message will only trigger on the JFK mezzanine, where we don't do passive readouts for four-car predictions

Since I didn't see JFK-specific logic in these changes, I'm guessing this is because of a general property of mezzanine signs, but I'm not quite connecting the dots. EDIT: covered in below comments!

Enum.take(message.predictions, if(multiple?, do: 1, else: 2))
four_cars? =
hd(message.predictions) |> PaEss.Utilities.prediction_four_cars?() and !multiple? and
!message.terminal?
Copy link
Contributor

@digitalcora digitalcora Feb 11, 2025

Choose a reason for hiding this comment

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

We're doing this same four_cars? and !multiple? and !terminal? conditional in two different places — I think it's actually a different struct in each, so maybe there's no easy way to avoid this, unless they already implement a common protocol/behaviour. (Maybe they should?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I thought about that, but the context is a bit different in each place, so there wasn't an obvious way to abstract it without making things less clear.

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.

All aboard the merge train 🚃🚃🚃🚃 (it has 4 cars. please move to the front)

I like how this turned out pretty straightforward, with the prior refactors in place!

@panentheos panentheos merged commit 2c1e848 into main Feb 13, 2025
2 checks passed
@panentheos panentheos deleted the bhw/four-cars branch February 13, 2025 14:31
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