Skip to content
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

Add phrases for post instruction processing #141

Merged
merged 4 commits into from
Sep 6, 2017
Merged

Add phrases for post instruction processing #141

merged 4 commits into from
Sep 6, 2017

Conversation

mcwhittemore
Copy link
Contributor

@mcwhittemore mcwhittemore commented Sep 1, 2017

There are times when one wants to link two instructions or prefix them with a message. These phrases should be translated with the same level of detail as the rest of this project, so I think they should live here. That said, I'm not quite sure how they should be added to the translation files so currently I've only added these to en.json.

This PR adds a phases to v5 of the translations along with these new continue phrases.

  • continue.default.distance
  • continue.default.namedistance
  • continue.straight.distance

Issue

I think this will close #88.

Tasklist

  • Add changelog entry
  • Review

@@ -50,6 +50,11 @@
"destination": "Take the ferry towards {destination}"
}
},
"phrase": {
"two linked by distance": "{instruction_one} then in {distance} {instruction_two}",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the spaces should be removed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any camel casing in here, so I've merged this like this.

two linked by distance => twolinkedbydistance

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcwhittemore retracting my comment, this should spaces.

@bsudekum
Copy link

bsudekum commented Sep 5, 2017

@mcwhittemore what is your plan for creating two instructions as in {instruction} then {instruction}? Right now the API only accepts a single step.

"phrase": {
"twolinkedbydistance": "{instruction_one} then in {distance} {instruction_two}",
"twolinked": "{instruction_one} then {instruction_two}",
"oneindistance": "In {distance}, {instruction_one}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a comma here? "In 100 meters, turn right" or "In 100 meters turn right"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if you can drop the introductory clause it should be separated from the rest of the sentence by a comma. At least, that is how I remember @lyzidiamond explaining these things to me a few months back, but I'm pretty bad at grammar so I could very well be wrong here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly right @mcwhittemore! "In 100 meters, turn right" is the correct structure.

@mcwhittemore
Copy link
Contributor Author

mcwhittemore commented Sep 5, 2017

what is your plan for creating two instructions as in {instruction} then {instruction}? Right now the API only accepts a single step.

@bsudekum - I'm doing something like where merged is the output I'm interested in.

var osrmTextInstructions = require('osrm-text-instructions');
var osrmEnglish = require('osrm-text-instructions/languages/translations/en.json');
var steps = [ ... ];

var one = osrmTextInstructions.complie('v5', { language: 'en', step: steps[0] }).toLowerCase();
var two = osrmTextInstructions.complie('v5', { language: 'en', step: steps[1] }).toLowerCase();

var opts = {
  instruction_one: one,
  instruction_two: two
};

var tokenString = osrmEnglish['v5']['phrase']['twolinked'];

var merged = osrmTextInstructions.tokenize(tokenString, opts, 'en');

@yuryleb
Copy link
Contributor

yuryleb commented Sep 6, 2017

BTW there are some issues with instructions combine this way:

  • All phrases in text resources should start with non-capital letter (maybe except German) to be ready to insert into combined instruction at any position
  • Even this doesn't help now - osrmTextInstructions.compile() will capitalize first letter by default (see meta.capitalizeFirstLetter option in language JSON files) for each instruction again
  • What's about translate strings for {distance}? Meters/miles question is interesting too 😉

@mcwhittemore
Copy link
Contributor Author

mcwhittemore commented Sep 6, 2017

Thanks for the feedback @yuryleb!

All phrases in text resources should start with non-capital letter

Ah. Good point. I've updated my example above to use toLowerCase() to convert the result of compile into a string that will work for tokenize. That said, this is rather complicated across languages and, as of now, is not something my PR tries to help the end user solve.

What's about translate strings for {distance}

This is a good idea. I'm going to add constants.distanceunits which will be an object of words like feet, meter and so on.

@mcwhittemore
Copy link
Contributor Author

Hmm... it looks like all constants have to be translated before tests will pass. Is this by design?

@yuryleb
Copy link
Contributor

yuryleb commented Sep 6, 2017

How distance value will be rounded before formatting? It will be language-dependent rule/expression (for English always in miles/feet, in kilometers/meters otherwise)?

Also I hope final distance value will be rounded to 10 😉 Otherwise I expect "plural" hell with Russian distance units translation and also with some other languages.

@mcwhittemore
Copy link
Contributor Author

How distance value will be rounded before formatting?

This PR is pushing this question to the down stream user and requiring them to know if they need the plural or non-plural word per the language being used.

Also I hope final distance value will be rounded to 10 😉 Otherwise I expect "plural" hell with Russian distance units translation and also with some other languages.

Can you explain this a bit more? Are you saying you hope 1.3 meters is rounded to 10 meters?

@yuryleb
Copy link
Contributor

yuryleb commented Sep 6, 2017

I meant there are 3 forms of quantity in Russian not only "one" and "many". For example "In xxx meters do something ..." in Russian:

  • Через 21 метр ...
  • Через 22 метра ...
  • Через 23 метра ...
  • Через 24 метра ...
  • Через 25 метров ...
  • the same up to 30 and again (but 11-20 values have the same plural quantity)

But if distance will be always rounded to 10, the only one plural form could be used in all languages AFAIK.

Also how precise should be the distance? Again for example Garmin navigators have limited set of similar distance instructions: for 100 m, 200 m and so on till 800 m, than for 1 km, 1.5 km (BTW single word numeral in Russian) and 2 km to 8 km. I don't propose to follow exactly this way but some similar distance rounding should be used. And if distance is less than 10 meters (or better 100 meters for drivers), it should be excluded at all.

@mcwhittemore
Copy link
Contributor Author

@yuryleb - thanks for your feedback. For now I'm going to drop unit translation support from this PR and, as a next step, start looking for a javascript lib that does this well already. My hope is that we find one this is really good. If its missing languages that we support, we can then push that common lib forward rather then writing our own.

@mcwhittemore
Copy link
Contributor Author

Oh, as a final note, the phrases currently should avoid plurality in translation as they expect the client to pass them a translated distance string.

- Adds three phrases to the English translations for combining
  instructions and referencing instruction in relation to distance.
- Adds `distance` support to the English `continue.default` and
  `continue.straight` instructions.
Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@1ec5
Copy link
Member

1ec5 commented Sep 7, 2017

Opened Project-OSRM/osrm-text-instructions.swift#37 to track a Swift port of this work.

@1ec5
Copy link
Member

1ec5 commented Sep 7, 2017

Since the iOS navigation SDK has historically vended its own phrase format strings, we can copy some of the translations over in Transifex from this resource to this resource.

@1ec5 1ec5 mentioned this pull request Sep 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distance-based instructions
6 participants