Add waypoint arrival instruction #111

bsudekum · 2017-06-09T21:52:44Z

This adds instructions for arriving at waypoints and also fixes an issue with updating fixtures.

Necessary for mapbox/mapbox-navigation-ios#270

bsudekum · 2017-06-09T22:03:14Z

Note, I had to add a metadata field to the fixture files to configure each compile test to allow for unique legIndex values in d0f94ad

bsudekum · 2017-06-09T22:03:51Z

index.js

@@ -68,7 +68,7 @@ module.exports = function(version, _options) {

            return config.join('');
        },
-        compile: function(language, step) {
+        compile: function(language, step, legIndex) {


New optional third argument, legIndex.

How would you indicate “final”? −1? (We would also need to add a string for “final” to the translations.)

1ec5 · 2017-06-09T22:10:00Z

index.js

@@ -173,7 +172,7 @@ module.exports = function(version, _options) {
                .replace('{lane_instruction}', laneInstruction)
                .replace('{modifier}', instructions[language][version].constants.modifier[modifier])
                .replace('{direction}', this.directionFromDegree(language, step.maneuver.bearing_after))
-                .replace('{nth}', nthWaypoint)
+                .replace('{nth}', legIndex && legIndex > 0 ? this.ordinalize(language, legIndex) : '')


According to this logic, if there are multiple destinations, the first destination is announced as, “You have arrived at your destination.”

I think it may be necessary to accept yet another parameter, the leg count. (That way we also don’t need to represent the last step index as −1.)

1ec5 · 2017-06-10T00:36:08Z

index.js

@@ -173,7 +175,7 @@ module.exports = function(version, _options) {
                .replace('{lane_instruction}', laneInstruction)
                .replace('{modifier}', instructions[language][version].constants.modifier[modifier])
                .replace('{direction}', this.directionFromDegree(language, step.maneuver.bearing_after))
-                .replace('{nth}', nthWaypoint)
+                .replace('{nth}', waypoint)


nthWaypoint was a more descriptive name. We aren't dealing with the waypoint itself; we're dealing with an ordinal word representing the one-based leg "index".

1ec5 · 2017-06-10T00:37:15Z

index.js

@@ -162,9 +163,10 @@ module.exports = function(version, _options) {
                instruction = options.hooks.tokenizedInstruction(instruction);
            }

+            var waypoint = legIndex >= 0 && legIndex !== legCount - 1 ? this.ordinalize(language, legIndex + 1) : '';


If it's the last of several waypoints, should we say "final destination"? The current code says only "destination".

Yes, but we currently do not have a translation for this.

This repository must have a source string before translators can provide translations in Transifex. Therefore, assuming that it’s a good idea to say “final destination”, this PR needs to add the “final destination” string to en.json. To avoid breaking the tests, I suppose we’d wait to make use of that string until enough translations are in.

freenerd

Therefore, assuming that it’s a good idea to say “final destination”, this PR needs to add the “final destination” string to en.json.

We used to have final destination and it sounded grim. Would last destination also work? Also we should make sure to not emit that if it's a one-leg route only.

freenerd · 2017-06-12T09:18:49Z

index.js

@@ -68,10 +68,11 @@ module.exports = function(version, _options) {

            return config.join('');
        },
-        compile: function(language, step) {
+        compile: function(language, step, legIndex, legCount) {


Can we make this optional? For most cases (read: non-multi waypoint cases) we will not need these properties. I'd be good to put them into an options object.

compile: function(language, step, options) {

freenerd · 2017-06-12T09:22:59Z

test/fixtures_test.js

            fs.writeFileSync(
                fileName,
                JSON.stringify({
                    step: step,
-                    instructions: instructionsForLanguages(step)
+                    instructions: instructionsForLanguages(step, legIndex, legCount),
+                    metadata: metadata


Do we even need to add metadata if it's just an empty object most of the time?

freenerd · 2017-06-12T09:23:22Z

test/fixtures/v5/arrive_waypoint/left.json

+        "name": "Street Name"
+    },
+    "instructions": {
+        "de": "Sie haben Ihr erste Ziel erreicht, es befindet sich links von Ihnen",


We'll need gendering of the adjective here 😢 Maybe we can get around this with some smart same-gender naming, I'll have a look ...

Now: Sie haben Ihr erste Ziel erreicht.
Correct: Sie haben Ihr erstes Ziel erreicht.

bsudekum · 2017-06-12T18:20:45Z

@freenerd I think having final makes sense now that we have in between waypoints. Thoughts @1ec5?

1ec5 · 2017-06-12T20:20:34Z

Haha, good point about “final destination”. 🙀 In an ideal world, we’d have a name for each waypoint that we’d say instead of “your destination”, but I suppose “your last destination” is passable.

(The railfan in me would have it say “last scheduled station stop”. 😏)

freenerd · 2017-06-13T08:56:08Z

Readme and Changelog still need fixups.

bsudekum · 2017-06-13T18:53:13Z

@freenerd will update the changelog when I update the package's version.

bsudekum · 2017-06-15T16:32:32Z

@freenerd @1ec5 want to give this another review?

freenerd · 2017-06-19T12:30:32Z

index.js

@@ -68,7 +68,7 @@ module.exports = function(version, _options) {

            return config.join('');
        },
-        compile: function(language, step) {
+        compile: function(language, step, legOptions) {


should we keep this with options only? That would be consistent with the options we already have for the module initialization.

freenerd · 2017-06-19T12:34:06Z

test/fixtures/v5/arrive_waypoint_last/uturn.json

+        "vi": "Đến nơi ",
+        "zh-Hans": "您已经到达您的个目的地"
+    },
+    "metadata": {


this should become options then as well

bsudekum · 2017-06-19T13:06:06Z

@freenerd note, while changing legOptions -> options, I had to change another options variable to opts. Can be seen in cf67806

1ec5 · 2017-07-06T06:56:02Z

Necessary for mapbox/mapbox-navigation-ios#270

The iOS navigation SDK uses the Swift port of this library for logic such as what’s in this PR. Tracking a Swift port in Project-OSRM/osrm-text-instructions.swift#24.

Bobby Sudekum added 2 commits June 9, 2017 14:49

Add waypoint arrival insrtruction

3bddf00

lint

4f898c0

bsudekum requested a review from freenerd June 9, 2017 21:52

Add metadata field to fixture

d0f94ad

bsudekum commented Jun 9, 2017

View reviewed changes

1ec5 reviewed Jun 9, 2017

View reviewed changes

Bobby Sudekum added 4 commits June 9, 2017 16:14

Add legCount

8a1d8ad

test arguments

294e63e

remove

7738042

Fix

c8b0024

1ec5 reviewed Jun 10, 2017

View reviewed changes

Better var name

b4f724b

freenerd requested changes Jun 12, 2017

View reviewed changes

Bobby Sudekum added 2 commits June 12, 2017 11:50

simplify

b879b41

remove

2a52c05

simplify

34bfb37

Update readme

909118a

freenerd mentioned this pull request Jun 15, 2017

Split arrival instruction into counting and non-counting #110

Closed

freenerd reviewed Jun 19, 2017

View reviewed changes

freenerd approved these changes Jun 19, 2017

View reviewed changes

change arg to options

cf67806

bsudekum merged commit 2ad8d70 into master Jun 19, 2017

bsudekum deleted the waypoint-arrive branch June 19, 2017 13:09

1ec5 mentioned this pull request Jul 6, 2017

Count waypoint upon arrival Project-OSRM/osrm-text-instructions.swift#24

Closed

1ec5 mentioned this pull request Jul 27, 2017

handle waypoints #7

Closed

hdaymon mentioned this pull request Sep 8, 2017

Ordinalize to 25 #78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add waypoint arrival instruction #111

Add waypoint arrival instruction #111

bsudekum commented Jun 9, 2017

bsudekum commented Jun 9, 2017 •

edited

bsudekum Jun 9, 2017 •

edited

1ec5 Jun 9, 2017

1ec5 Jun 9, 2017

1ec5 Jun 9, 2017

1ec5 Jun 10, 2017

1ec5 Jun 10, 2017

bsudekum Jun 11, 2017

1ec5 Jun 12, 2017

freenerd left a comment

freenerd Jun 12, 2017

freenerd Jun 12, 2017

freenerd Jun 12, 2017

bsudekum commented Jun 12, 2017

1ec5 commented Jun 12, 2017

freenerd commented Jun 13, 2017

bsudekum commented Jun 13, 2017

bsudekum commented Jun 15, 2017

freenerd Jun 19, 2017

freenerd Jun 19, 2017

bsudekum commented Jun 19, 2017

1ec5 commented Jul 6, 2017

Add waypoint arrival instruction #111

Add waypoint arrival instruction #111

Conversation

bsudekum commented Jun 9, 2017

bsudekum commented Jun 9, 2017 • edited

bsudekum Jun 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freenerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Jun 12, 2017

1ec5 commented Jun 12, 2017

freenerd commented Jun 13, 2017

bsudekum commented Jun 13, 2017

bsudekum commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Jun 19, 2017

1ec5 commented Jul 6, 2017

bsudekum commented Jun 9, 2017 •

edited

bsudekum Jun 9, 2017 •

edited