Port of osrm-text-instructions to Java#374
Conversation
|
@zugaldia, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cammace, @ivovandongen and @ghoshkaj to be potential reviewers. |
| libosrm: | ||
| rm -rf mapbox/libjava-services/src/main/resources/translations | ||
| mkdir -p mapbox/libjava-services/src/main/resources/translations | ||
| cp ../osrm-text-instructions/languages/translations/* mapbox/libjava-services/src/main/resources/translations |
There was a problem hiding this comment.
This assumes the developer has a clone of osrm-text-instructions at a specific location on disk, and people with different commits checked out will end up with different JSON files in this repository. Can we make this dependency less fragile, perhaps using a submodule?
There was a problem hiding this comment.
If you check in the JSON files, then the submodule can be something only contributors to this project have to worry about; someone incorporating the library would use the checked-in JSON files but never have to worry about how they got there.
There was a problem hiding this comment.
The current code’s disadvantage is twofold: it assumes a suprarepository file structure, and it doesn’t record versions or commit hashes, so it’ll be more difficult to bisect should something go wrong that depends on specific translations.
|
The good news is that the core logic as well as tests have now been ported. The bad news is that the tests for the
|
|
It looks like you've added additional constructors to some of the direction model classes. Would this resolve #368 or do we still want to add setters? |
zugaldia
changed the title
osrm-text-instructions to Javaosrm-text-instructions to Java
4 participants
For easier iterations, let's include it in the navigation package. Once it's stable, let's spin it off as its own package / repo possibly like https://github.com/Project-OSRM/osrm-text-instructions.swift.
cc: @freenerd @frederoni @bsudekum @1ec5 @cammace