-
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
🧠 Ingest OIO trigger and suppress relevant predictions #351
Conversation
Coverage of commit
|
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.
Overall looks good, just a few minor thoughts that are non-blocking.
@@ -74,8 +77,8 @@ config :concentrate, | |||
Concentrate.GroupFilter.VehicleStopMatch, | |||
Concentrate.GroupFilter.SkippedStopOnAddedTrip, | |||
Concentrate.GroupFilter.TripDescriptorTimestamp, | |||
Concentrate.GroupFilter.UncertaintyValue | |||
# Concentrate.GroupFilter.SuppressStopTimeUpdate Enable once fully implemented |
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.
nit: I'm ambivalent personally, but there seems to be a preference here towards avoiding commented code. Maybe this should just be whacked, and then added when it's actually in use.
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.
this was actually already merged in and is being updated in this PR
Supervisor.start_link( | ||
[ | ||
{ | ||
Concentrate.producer_for_url(config[:url]), |
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.
Would it be worth adding a test to either confirm that it actually starts pulling this file?
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.
sure thing, focusing on getting the terraform changes in so that I can put this on dev for more thorough testing but can add a test for this
Coverage of commit
|
end) | ||
end | ||
|
||
defp parse_response(body, state) 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.
@nlwstein this was added so that the events list wasn't empty if the new state should be empty. otherwise the GenStage
does not actually pass along the empty array as a message.
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
Summary of changes
Asana Ticket: Fetch Config