-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: don't include non-revenue trips in feeds #323
Conversation
Coverage of commit
|
2db12cf
to
1668a38
Compare
Coverage of commit
|
1668a38
to
4d04f32
Compare
Coverage of commit
|
4d04f32
to
ff57584
Compare
This has been updated to use the (soon to be) newly added revenue field in RTR rather than filtering on |
Coverage of commit
|
%{route_pattern_id: TripDescriptor.route_pattern_id(update)} | ||
%{ | ||
route_pattern_id: TripDescriptor.route_pattern_id(update), | ||
revenue: TripDescriptor.revenue(update) |
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.
suggestion: given that true
is the default, what about only including it when the value is false
?
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.
If you feel strongly about it, I can. My main hesitation is that in nil
~ falsey languages, the naive interpretation is the opposite of what we actually mean. I'd tend to argue there shouldn't be a default, but maintaining current behavior on existing feeds kind of forces our hand there.
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.
As you note, existing clients which care about this value will need to treat a missing value as true
(as it was true for all trips before). But I don't think it needs to block the PR.
@@ -19,7 +21,7 @@ defmodule Concentrate.Encoder.VehiclePositionsEnhanced do | |||
end | |||
|
|||
def build_entity({%TripDescriptor{} = td, vps, _stus}) do | |||
trip = trip_descriptor(td) | |||
trip = trip_descriptor(td) |> Map.put("revenue", TripDescriptor.revenue(td)) |
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.
trip = trip_descriptor(td) |> Map.put("revenue", TripDescriptor.revenue(td)) | |
trip = td | |
|> trip_descriptor() | |
|> Map.put("revenue", TripDescriptor.revenue(td)) |
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.
We might want to consider turning on Credo's PipeChainStart
check. It's explicitly off at the moment.
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.
ff57584
to
d3ef98e
Compare
Coverage of commit
|
Summary of changes
Asana Ticket: 🧠 Concentrate suppresses non-revenue trips until December 7
Note: We will shortly want to start exposing non-revenue trips in the enhanced feed. This implementation is chosen to make that update trivial.