-
Notifications
You must be signed in to change notification settings - Fork 53
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
Exception based scheduling #545
Conversation
@binh-dam-ibigroup before i dive into a fix (to replace the temp fix that doesn't work) do you have any thoughts on how to approach this which might save me a bit of time?! |
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.
@br648 I think this is headed in the right direction! (Add the tests for checking that the logic works.)
src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/MergeLineContext.java
Outdated
Show resolved
Hide resolved
"%s:%s:%s", | ||
table.name, | ||
field.name, | ||
field.referenceTables.stream().map(r -> r.name).collect(Collectors.joining(":")) |
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.
Hopefully we don't rely extract values from keys by relying on the ":" separator.
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've made changes to this in my latest update to use a more unque key.
…dle multi ref tables
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.
Works well when merging two normal feeds!
src/main/java/com/conveyal/datatools/manager/jobs/feedmerge/ReferenceTableDiscovery.java
Outdated
Show resolved
Hide resolved
@br648 Remember to address the merge conflict. |
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 is looking good to me, although I think we should probably hold off on merging until we've got a front end fix for full testing.
Awaiting #567 to be merged |
key now includes reference to schedule exceptions matching trip service_id references
Checklist
dev
before they can be merged tomaster
)Description
This PR includes the exception based scheduling updates made to GTFS-lib (conveyal/gtfs-lib#385) and a rector of feed merge to accommodate multiple table references.