Refactor for more data-driven approach #27
Conversation
This makes translations possible and implementations in other languages easier on the cost of blowing up the instructions.json
| default: | ||
| case 'rotary': | ||
| case 'roundabout': | ||
| if (type === 'rotary' || type === 'roundabout') { |
There was a problem hiding this comment.
This if statement is redundant to the cases we're in.
| .replace('{destination}', (step.destinations || '').split(',')[0]) | ||
| .replace('{exit_number}', ordinalize(step.maneuver.exit || 1)) | ||
| .replace('{rotary_name}', step.rotary_name) | ||
| .replace('{laneInstruction}', laneInstruction) |
There was a problem hiding this comment.
Most of the tokens use train_case, but this one uses camelCase.
| "destination": "Continue straight towards {destination}" | ||
| }, | ||
| "left": { | ||
| "default": "{laneInstruction} to turn left", |
There was a problem hiding this comment.
For the purpose of minimizing translation cost, it may be desirable at some point to refactor this so that there would literally be a string {laneInstruction} to {instruction}. I'm not aware of any languages that would need more granularity in this case, but I could be wrong.
| "sharp right": { | ||
| "default": "Take a sharp right", | ||
| "name": "Take a sharp right onto {way_name}", | ||
| "destination": "Take a sharp right towards {way_name}" |
| "defaultInstruction": "Continue {modifier}[ onto {way_name}]" | ||
| "default": { | ||
| "default": "Continue {modifier}", | ||
| "name": "Continue {modifier} onto {way_name}" |
There was a problem hiding this comment.
Is it impossible to have a destination here?
|
Some other thoughts:
I think there are some good moves here going on but with this PR we're 3x'ing the instruction.json and only reducing |
|
Incorporated all inline comments in f54c828
Yes. It feels like the test fixtures need to eventually look similar to the instructions.json. I might also want to generate them programatically then. Specifically lanes, rotary and roundabouts will need work.
👍 |
In general, that's to be expected with making anything more data-driven, although I've suggested a change above that would reduce the string bloat a bit. |
freenerd
force-pushed
the
destinations-without-template
branch
from
f54c828 to
f9b110d
Compare
freenerd
force-pushed
the
destinations-without-template
branch
from
f9b110d to
be80c99
Compare
| "name": "{lane_instruction} to make a U-turn onto {way_name}", | ||
| "destination": "{lane_instruction} to make a U-turn towards {destination}" | ||
| }, | ||
| "lane_types": { | ||
| "xo": "Keep right", | ||
| "ox": "Keep left", |
|
On iOS, I was planning to use the Mapbox iOS/macOS SDK's MGLCompassDirectionFormatter class for compass directions (which would be localized along with the SDK), but this is a reasonable approach. |
|
Consolidated I think we are good here now, next step is merging, releasing a new version, integrating and then continue to fix things. Will do that tomorrow. |
freenerd
force-pushed
the
destinations-without-template
branch
from
d45dd61 to
cba0aae
Compare
Improves #23
Fixes #6
Ref #19 #24 #14 #8
This makes natural language translations possible and implementations in other programming languages easier on the cost of blowing up the instructions.json.
Next: