-
Notifications
You must be signed in to change notification settings - Fork 18
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
🍎🚫💰 Surface prediction 'uncertainty' values through v3 API #698
Conversation
Coverage of commit
|
question: is there a reason to ship this before we're providing the data in Concentrate? |
No particular reason. The concentrate portion is set to be done this sprint and can wait until that is out before merging/deploying this. |
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.
A couple of random thoughts but overall this looks good! 🚀
@@ -54,6 +56,12 @@ defmodule Parse.TripUpdates do | |||
nil | |||
end | |||
|
|||
defp parse_uncertainty(%{uncertainty: uncertainty}) when is_integer(uncertainty) do |
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.
Since your documentation (nice!) only allows three values, would it make sense to only allow those three values through? I think you can use a guard like when is_integer(uncertainty) and uncertainty in [60, 120, 360]
but I haven't tested 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.
Does this incorporate the uncertainty values that Swiftly uses?
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.
I could be mistaken, but I think those values are converted into the 60, 120, 360 versions before they make it here. I'll look into that to verify.
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.
Looks like these values are only rail, the swiftly values, which are different, cover bus/CR. Will update docs/types to address.
@@ -261,6 +261,34 @@ defmodule ApiWeb.PredictionController do | |||
example: "2017-08-14T15:38:58-04:00" | |||
) | |||
|
|||
arrival_uncertainty( |
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.
Does this get sucked into Swagger? I can't remember, but if it doesn't, might be worth outputting this to the docs wherever this field shows up.
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.
Yes, these show up in the models section at the bottom of the swagger docs
@nlwstein @paulswartz moved swagger docs into separate PR to include other non-rev documentation here: #700 |
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
Summary of changes
Asana Ticket: 🍎🚫💰 Surface prediction 'uncertainty' values through v3 API
arrival_uncertainty
anddeparture_uncertainty
attributes toPrediction
resource.example response: