Add fallback code for unsupported locales #168

bsudekum · 2017-10-03T22:04:48Z

This PR adds support for trying to find locales which we currently do not support. This gist of it is, that if a developer provides es-MX, we should give them instructions provides by es.

This also moves to use en when a language is not found.

/cc @mcwhittemore @1ec5

mcwhittemore

Can we change findAppropriateLanguage to findTranslation and have this return the translation object rather than a string which is a key in that index?

Doing this would let us do something like _.merge({}, englishTranslation, foundTranslation) and remove all the if not in the lang, look in english checks.

We could expose the language code selected on this object as well so that someone can use this function to know if their language code is supported by OSRM text instructions.

1ec5 · 2017-10-04T16:47:06Z

test/index_test.js

+                modifier: 'left'
+            },
+            name: 'Way Name'
+        }), 'Gire a la izquierda en Way Name');


turn left instructions are identical between es and es-ES. Can we find an instruction that differs between the two locales?

Actually, maybe it’d be better to unit test getAvailableLanguage() directly instead of (or in addition to) testing its integration as part of compile().

1ec5 · 2017-10-04T18:11:37Z

index.js

@@ -210,6 +211,30 @@ module.exports = function(version, _options) {
            }

            return output;
+        },
+        findAppropriateLanguage: function(language) {


Does this method need to be exported? Also, how about calling it getAvailableLanguage() or getBestMatchingLanguage()?

1ec5 · 2017-10-04T18:12:20Z

index.js

-            if (languages.supportedCodes.indexOf(language) === -1) throw new Error('language code ' + language + ' not loaded');
+        compile: function(originalLanguage, step, options) {
+            if (!originalLanguage) throw new Error('No language code provided');
+            var language = this.findAppropriateLanguage(originalLanguage);


Several other exported methods take a language parameter as well. We should be consistent; otherwise, a client using the tokenize() method, for example, will end up with a mismash.

1ec5 · 2017-10-04T18:14:18Z

index.js

@@ -210,6 +211,30 @@ module.exports = function(version, _options) {
            }

            return output;
+        },
+        findAppropriateLanguage: function(language) {
+            if (languages.supportedCodes.indexOf(language) > -1) {


languages.instructions is an object with language codes as the keys. I think it’d be a bit easier to work with that object than the supported codes array.

Let's keep the list of languages we're using here consistent. An array of locales is little more accurate than a dictionary with keys IMO.

1ec5 · 2017-10-04T18:23:57Z

index.js

+                // no country code needed.
+                return filteredSupportedLanguages.sort(function(a, b) {
+                    return a.length - b.length;
+                })[0];


I think we should look for matches in this order (prior art), at least until we can pull in a more sophisticated library that specializes in language fallback logic:

Same full locale code (e.g., lng-Scpt-CC)

Same language code and script code (lng-Scpt)

Same language code and country code (lng-CC)

Same language code (lng)

Same language code and any script code (lng-Scpx)

Same language code and any country code (lng-CX)

English (en)

It’ll help if we use languages.instructions instead of languages.supportedCodes.

/cc @jcsg

I think that sounds good. I was originally worried that condition 5 might mean that we'd serve Serbian in Latin rather than Cyrillic, but that probably would be better than just serving English for that region, especially if the voice instructions still use Serbian.

Thanks for looping me in, @1ec5 .

1ec5 · 2017-10-04T19:41:32Z

Can we change findAppropriateLanguage to findTranslation and have this return the translation object rather than a string which is a key in that index?

We could simplify this PR considerably by removing all the calls to getBestMatchingLanguage() and making the client responsible for calling getBestMatchingLanguage() before passing a locale code into any other method like compile() or tokenize(). To enforce that, this library would throw an error if the passed-in locale code isn’t available.

1ec5 · 2017-10-04T20:10:30Z

index.js

+
+            // Same language code (lng)
+            } else if (supportedLanguageCode.indexOf(languageCode) > -1) {
+                return languages.supportedCodes[supportedLanguageCode.indexOf(languageCode)];


This entire expression is equivalent to just languageCode.

1ec5 · 2017-10-04T20:12:11Z

index.js

+                return languageCode + '-' + countryCode;
+
+            // Same language code (lng)
+            } else if (supportedLanguageCode.indexOf(languageCode) > -1) {


This is guaranteed to be equivalent to if (language.instructions[languageCode]).

1ec5 · 2017-10-04T20:16:38Z

index.js

+                countryCode = splitLocale[2];
+            }
+
+            var supportedLanguageCode = languages.supportedCodes.map(function(locale) {


This is an array, so the name should be plural.

1ec5 · 2017-10-04T20:17:26Z

index.js

+                return languages.supportedCodes[supportedLanguageCode.indexOf(languageCode)];
+
+            // Same language code and any script code (lng-Scpx)
+            } else if (supportedLanguageCode.indexOf(languageCode) > -1 && scriptCode) {


If we support zh-Hans but not zh, and language is zh-Hant, then we won’t go into this if statement because zh-Hant isn’t one of the supported languages. Instead, we need to look for an item in supportedLanguageCodes that contains the language code zh and also contains a script code.

Addressed in 167f7c2

1ec5 · 2017-10-05T01:33:09Z

test/index_test.js

+    });
+
+    t.test('getBestMatchingLanguage', function(t) {
+        t.assert(compiler('v5').getBestMatchingLanguage('zh-Hant'), 'zh-Hans');


Let’s make sure all the cases above get proper unit tests too, especially es-MX → es. It’ll also be important to make sure we match the language codes case-insensitively, since that’s what the Directions API does.

1ec5 · 2017-10-05T02:04:50Z

index.js

+            var scriptCode = false;
+            var countryCode = false;
+
+            if (splitLocale.lenth === 1) {


Typo: length.

1ec5 · 2017-10-05T02:06:13Z

index.js

+            var countryCode = false;
+
+            if (splitLocale.lenth === 1) {
+                languageCode = splitLocale[0];


Might as well set languageCode unconditionally at this point. If language is something bogus like c++, then we’ll end up falling back to English anyways.

I'm going to keep the splitting code and the return code separated.

There’s no else case here. If language is the empty string, then we wind up looking up silly values like false-false in instructions. I continue to think we should pull this line out of the if and set it unconditionally.

1ec5 · 2017-10-05T02:07:49Z

index.js

+            } else if (splitLocale.length === 3) {
+                languageCode = splitLocale[0];
+                scriptCode = splitLocale[1];
+                countryCode = splitLocale[2];


Here’s a useful reference on the syntax of a BCP47 language tag. We’re currently handling all the parts that a client is likely to pass in.

1ec5 · 2017-10-05T02:09:32Z

index.js

+            // Same language code and any script code (lng-Scpx) and the found language contains a script
+            } else if (supportedLanguageCodes.indexOf(languageCode) > -1 && scriptCode &&
+                languages.supportedCodes[supportedLanguageCodes.indexOf(languageCode)].split('-').length > 1 &&
+                languages.supportedCodes[supportedLanguageCodes.indexOf(languageCode)].split('-')[1].length === 4) {


This would be easier if, right off the bat, we map each item in supportedCodes to an object with the keys language, script, and region.

It'd only be used really for this case. I'm fine with keeping this.

If we do the preprocessing suggested above, then this becomes:

else if (availableLanguages.find(function (language) { return language.language === languageCode && language.script; }))

and some of the duplication in the next else if block goes away.

We can do that preprocessing inside languages.js to avoid any meaningful performance penalty.

Changed it around to this in 9a19e1e, not sure if it's any more clear.

1ec5 · 2017-10-05T19:54:24Z

index.js

+            })) {
+                return languages.supportedCodes[supportedLanguageCodes.indexOf(languageCode)];
+            // Same language code and any country code (lng-CX)
+            } else if (supportedLanguageCodes.indexOf(languageCode) > -1 && countryCode) {


If the passed-in language is pt-PT and we support pt-BR (but not pt), this line checks whether pt-PT is supported (it isn’t) and pt-PT has a country code (it does). Consequently, we fail to go in here. Let’s add a test for this case.

added here: https://github.com/Project-OSRM/osrm-text-instructions/pull/168/files#diff-979945c097652b2859f6887cda2f7178R282

1ec5 · 2017-10-05T19:55:48Z

index.js

+            });
+
+            var availableLanguages = languages.supportedCodes.map(function(language) {
+                return that.divideLanguageCodes(language);


languages.js currently exports a supportedCodes containing strings; it should also export supportedLanguages (or availableLanguages) that maps to these objects. That way this code only has to run once, no matter how many times getBestMatchingLanguage() is called.

1ec5 · 2017-10-05T19:57:35Z

index.js

+            } else if (availableLanguages.find(function (language) {
+                return language.languageCode === languageCode && language.scriptCode;
+            })) {
+                return languages.supportedCodes[supportedLanguageCodes.indexOf(languageCode)];


The object returned by find() should have a locale property that contains the full string, so that we can just return the locale property here instead of searching through supportedCodes.

1ec5 · 2017-10-05T20:08:45Z

index.js

+        },
+        divideLanguageCodes: function(language) {
+            // If the language is not found, try a little harder
+            var splitLocale = language.toLowerCase().split('-');


To me, it’d be cleaner to use a regular expression for this job:

// Two-letter language code, then optional four-letter script code, then optional two-letter region code var match = language.match(/(\w\w)(?:-(\w\w\w\w))?(?:-(\w\w))?/i); // Normalize case var locale = []; if (match[1]) { match[1] = match[1].toLowerCase(); locale.push(match[1]); } if (match[2]) { match[2] = match[2][0].toUpperCase() + match[2].substring(1).toLowerCase(); locale.push(match[2]); } if (match[3]) { match[3] = match[3].toUpperCase(); locale.push(match[3]); } return { locale: locale.join("-"), language: match[1], script: match[2], region: match[3] };

Looks like a lot, but most of it is to normalize the case, which is important for avoiding having to index into supportedLanguages all the time.

This is failing for me. I think both styles of parsing out the codes is fine.

If it’s failing, it’s probably because the rest of the code hasn’t been updated to account for the normalized case being used by ☝️.

1ec5 · 2017-10-05T20:09:45Z

test/index_test.js

+        t.assert(compiler('v5').getBestMatchingLanguage('es-MX'), 'es');
+        t.assert(compiler('v5').getBestMatchingLanguage('es-es'), 'es-es');
+        t.end();
+    });


Let’s add some tests of divideLanguageCodes() or whatever we call it once we move it to languages.js.

Added here: https://github.com/Project-OSRM/osrm-text-instructions/pull/168/files#diff-979945c097652b2859f6887cda2f7178R288

1ec5 · 2017-10-05T22:32:43Z

index.js

+            })) {
+                return languages.parsedSupportedCodes.find(function (language) {
+                    return language.languageCode === languageCode && language.scriptCode;
+                }).locale;


Instead of repeating the find() call, store the result of the first find. It’d be a lot easier to store things in local variables if you replace all the else ifs with ifs – after all, we’re returning early in each case.

1ec5 · 2017-10-05T22:36:27Z

test/index_test.js

+        t.deepEqual(languages.parseLanguageIntoCodes('zh'), { countryCode: false, languageCode: 'zh', locale: 'zh', scriptCode: false });
+        t.deepEqual(languages.parseLanguageIntoCodes('es-MX'), { countryCode: 'mx', languageCode: 'es', locale: 'es-MX', scriptCode: false });
+        t.deepEqual(languages.parseLanguageIntoCodes('es-ES'), { countryCode: 'es', languageCode: 'es', locale: 'es-ES', scriptCode: false });
+        t.deepEqual(languages.parseLanguageIntoCodes('pt-PT'),  { countryCode: 'pt', languageCode: 'pt', locale: 'pt-PT', scriptCode: false });


The Directions API currently allows a language parameter to be specified in any case, such as pt-pt or PT-Pt. If the Directions API is to use getBestMatchingLanguage(), we should test that this method matches case insensitively, so pt-pt should also produce pt-BR.

1ec5 · 2017-10-05T22:38:09Z

languages.js

+        locale: language,
+        languageCode: languageCode,
+        scriptCode: scriptCode,
+        countryCode: countryCode


It’s a bit redundant to say “code” in each of these keys.

bsudekum mentioned this pull request Oct 4, 2017

Use voice instructions from server mapbox/mapbox-navigation-ios#614

Merged

3 tasks

mcwhittemore reviewed Oct 4, 2017

View reviewed changes

1ec5 reviewed Oct 4, 2017

View reviewed changes

bsudekum force-pushed the fallback branch from c8c9fc2 to 1eaa306 Compare October 4, 2017 18:30

Bobby Sudekum added 2 commits October 4, 2017 11:50

Add fallback code for unsupported locales

6b3afea

fix lint

4d952e8

bsudekum force-pushed the fallback branch from d277b37 to 4d952e8 Compare October 4, 2017 18:50

improve

46af76e

fix

5cedb00

1ec5 reviewed Oct 4, 2017

View reviewed changes

Bobby Sudekum added 6 commits October 4, 2017 13:32

Update

91bba84

Update name, add unit test

a7e10aa

remove lint

e46bea7

Strengthen script

167f7c2

stronger

2f40ceb

Update changlog

46a02f0

1ec5 reviewed Oct 5, 2017

View reviewed changes

Bobby Sudekum added 3 commits October 5, 2017 09:13

more

9580c37

improve

9a19e1e

language

fb33367

1ec5 reviewed Oct 5, 2017

View reviewed changes

Bobby Sudekum added 5 commits October 5, 2017 14:37

move

86c0761

export

6fcd345

use find

f69f3c3

more test

2550759

Use regex

72ea895

1ec5 reviewed Oct 5, 2017

View reviewed changes

Bobby Sudekum added 2 commits October 5, 2017 15:41

pt-pt

1cfe155

fix

5237e12

1ec5 added 5 commits October 5, 2017 16:05

Simplified language matching

e4d58a1

Simplified properties in language objects

e605e82

Eliminated extra map when matching languages

d5b6331

Moved language tests to the right file

3684ed3

Copyedited changelog and readme

d32fbc9

1ec5 approved these changes Oct 5, 2017

View reviewed changes

1ec5 merged commit 7de0b6c into master Oct 5, 2017

1ec5 deleted the fallback branch October 5, 2017 23:12

bsudekum mentioned this pull request Oct 6, 2017

Add language/locale mapbox/mapbox-directions-swift#176

Merged

1ec5 mentioned this pull request Nov 9, 2017

Default to current locale mapbox/mapbox-directions-swift#201

Closed

Add fallback code for unsupported locales #168

Add fallback code for unsupported locales #168

Conversation

bsudekum commented Oct 3, 2017

mcwhittemore 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Oct 4, 2017

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

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

1ec5 Oct 5, 2017 • edited Loading

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

1ec5 Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Oct 5, 2017 • edited Loading

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

1ec5 Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Oct 4, 2017 •

edited

Loading

1ec5 Oct 5, 2017 •

edited

Loading

1ec5 Oct 5, 2017 •

edited

Loading

1ec5 Oct 5, 2017 •

edited

Loading

1ec5 Oct 5, 2017 •

edited

Loading