Merge (current/future) feed versions for MTC #186
Conversation
Conflicts: src/main/java/com/conveyal/datatools/manager/controllers/api/FeedVersionController.java src/main/java/com/conveyal/datatools/manager/jobs/MergeProjectFeedsJob.java
…into merge-feed-versions-mtc
src/main/java/com/conveyal/datatools/manager/controllers/api/FeedVersionController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/controllers/api/ProjectController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java
Outdated
Show resolved
Hide resolved
src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java
Outdated
Show resolved
Hide resolved
|
I would really like to see some tests be made for the merging of feeds for both the MTC and REGIONAL merge types. |
Codecov Report
@@ Coverage Diff @@
## dev #186 +/- ##
==========================================
+ Coverage 6.19% 13.5% +7.31%
- Complexity 115 271 +156
==========================================
Files 133 146 +13
Lines 6971 7254 +283
Branches 914 973 +59
==========================================
+ Hits 432 980 +548
+ Misses 6503 6182 -321
- Partials 36 92 +56
Continue to review full report at Codecov.
|
src/main/java/com/conveyal/datatools/manager/jobs/MergeFeedsJob.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Sorry for the crapton of comments, most of them are cosmetic and asking for more comments on stuff. However, some other things I think are important to address also like trying to find a way to not write entire GTFS tables to an in-memory byte[], detecting merge failures for when agency_ids don't match up, and correctly counting remapped references.
| mergeFeedsResult.failed = true; | ||
| mergeFeedsResult.errorCount++; | ||
| mergeFeedsResult.failureReasons.add( | ||
| "If one stops.txt file contains stop_codes, both feed versions must stop_codes."); |
There was a problem hiding this comment.
This error message could use some grammatical improvement.
| // The line already exists in the output file, do not append it again. This prevents duplicate | ||
| // entries for certain files that do not contain primary keys (e.g., fare_rules and transfers) and | ||
| // do not otherwise have convenient ways to track uniqueness (like an order field). | ||
| // FIXME: add ordinal field/compound keys for transfers (from/to_stop_id) and fare_rules (?). |
There was a problem hiding this comment.
We should make an issue for this if we aren't going to fix this right away.
| MergeFeedsType mergeType; | ||
| try { | ||
| mergeType = MergeFeedsType.valueOf(req.queryParams("mergeType")); | ||
| if (mergeType.equals(REGIONAL)) throw new IllegalArgumentException(); |
There was a problem hiding this comment.
Please populate the IllegalArgumentException with a string error.
There was a problem hiding this comment.
Hmm, I see that it's caught down below. Maybe it'd be good to specify that the REGIONAL type is not allowed for this endpoint.
|
🎉 This PR is included in version 3.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Checklist
devbefore they can be merged tomaster)mtc extension should be enabled.
Description
This PR resolves #185, by refactoring the merge project feeds job into a generic merge job that can handle different merge strategies. These strategies now include: REGIONAL (for merging feeds for a project) and MTC (which actualizes the logic described in #185).