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

add alert message variants #825

Merged
merged 1 commit into from
Oct 8, 2024
Merged

add alert message variants #825

merged 1 commit into from
Oct 8, 2024

Conversation

panentheos
Copy link
Collaborator

Summary of changes

This is a small refactor in advance of a larger one. Adds two new message variants:

  • A short version of the single-line destination-specific "no service" message, which can fit on the top line.
  • A destination-specific version of the full-page "no service" message.
    Note that neither of these messages are used yet, but they will be in a followup PR. The language has been ok'd by product.

Also reworks the line field in the "service ended" message as variant, to make it more general.

@panentheos panentheos requested a review from a team as a code owner October 4, 2024 19:56
@digitalcora
Copy link
Contributor

Just a note that I have looked at this but I'm not confident in my approval without test changes showing the difference in behavior — but then maybe those couldn't be included in this PR if no code actually uses the new messages yet. Maybe it makes sense to do the refactor and then the "real" task in the same PR, as separate commits?

@panentheos
Copy link
Collaborator Author

Yeah, this code is exercised by the extensive suit in realtime_test.exs, and the lack of diffs is what shows that this isn't used yet. I split out this and the other pieces to try to avoid putting up a very large PR, since they tend to be more burdensome to review, even if they're broken up in to logical chunks. The next PR will have some straightforward diffs in the test assertions that showcase the new messages.

@digitalcora
Copy link
Contributor

One approach we could take is to base the following PR on this branch. Then they are separate PRs, but the first one doesn't have to be merged before the second one can be reviewed. GitHub will even auto-retarget the second PR to main when the first one is merged. But again, this would mostly be to benefit my understanding.

@PaulJKim Would you mind taking a look here? I'm not as familiar with these internals as you two.

@panentheos
Copy link
Collaborator Author

Ok, I pushed #828 as a draft, so you can refer to it while reviewing this. I'll rebase that and write a full description once this one goes in.


@type t :: %__MODULE__{
destination: PaEss.destination(),
route: String.t() | nil,
line: :top | :bottom
Copy link
Collaborator

Choose a reason for hiding this comment

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

The 'line' pattern is also used in custom.ex. Should we go ahead and refactor that while we're at it?

Copy link
Collaborator Author

@panentheos panentheos Oct 8, 2024

Choose a reason for hiding this comment

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

That one's a bit different because we can't programmatically shorten or rearrange custom messages, like we can with these variants.

@PaulJKim
Copy link
Collaborator

PaulJKim commented Oct 8, 2024

I took a look at this as well and I think it would be fine to merge since the destination-specific messages are never actually used, as mentioned in the description. The change to the last trip stuff looks fine as well and just had a non-blocking comment related to that. I'll leave final approval to you @digitalcora since you initially reviewed it

@digitalcora
Copy link
Contributor

Ha, my intention was to leave the approval to you as the resident expert... so I shall approve based on your approval 😄

@panentheos panentheos merged commit e938068 into main Oct 8, 2024
2 checks passed
@panentheos panentheos deleted the bhw/prefactor-messages branch October 8, 2024 17:22
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.

3 participants