-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
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? |
Yeah, this code is exercised by the extensive suit in |
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 @PaulJKim Would you mind taking a look here? I'm not as familiar with these internals as you two. |
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 |
Ha, my intention was to leave the approval to you as the resident expert... so I shall approve based on your approval 😄 |
Summary of changes
This is a small refactor in advance of a larger one. Adds two new message variants:
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 asvariant
, to make it more general.