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

Fix language alias for http function #153

Merged
merged 1 commit into from Jan 28, 2020

Conversation

@codechennerator
Copy link
Contributor

codechennerator commented Jan 22, 2020

Purpose:
The purpose of this PR is to:

  1. Fix the test that was checking on the language aliases when http(req) is called.
  2. Fix when aliases work when g.http(req) is called.

Tests:
The tests that were in previously for aliases are appropriate for this PR. However, I modified the AppLanguages in order to correctly reflect a strong-globalize environment.

Explanation:
The original tests missed the scenario I'm running into now: The http function doesn't map the alias properly when the alias language is not in the appLanguages.

For example: Say my acceptedLanguages (which is determined by my folder structure) is cs, en, zh-Hans, zh-Hant.

g.http will not map properly to zh-Hans because zh-cn is not in the appLanguages. Instead, the acceptLanguage function will return the first language in the appLanguages list. (In this case, cs) When getLangAlias is called it simply matches cs with cs, but by then the language is wrong.

This was not caught in the test because in the test we included zh-cn as part of the appLanguages. This returns a result from accept-language that is not expected. See: here. Alias conversion should work if appLanguages = en, zh-Hans. Otherwise, the intl folder structure would be:

intl
├── en
├── zh-cn
├── zh-Hans

which is incorrect if zh-Hans is to equivocate to zh-cn.

@slnode

This comment has been minimized.

Copy link

slnode commented Jan 22, 2020

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

Copy link
Contributor

jannyHou left a comment

@codechennerator Your change make sense to me, good catch 👍

Could you fix the lint error?

> prettier "**/*.ts" "**/*.js" "-l"

packages/runtime/src/helper.ts

npm ERR! code ELIFECYCLE

npm ERR! errno 1

npm ERR! strong-globalize@4.0.0 prettier:cli: `prettier "**/*.ts" "**/*.js" "-l"`

npm ERR! Exit status 1

npm ERR! 

npm ERR! Failed at the strong-globalize@4.0.0 prettier:cli script.

npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:

npm ERR!     /home/travis/.npm/_logs/2020-01-22T02_04_36_673Z-debug.log

npm ERR! code ELIFECYCLE

npm ERR! errno 1

npm ERR! strong-globalize@4.0.0 prettier:check: `npm run prettier:cli -- -l`

npm ERR! Exit status 1

npm ERR! 

npm ERR! Failed at the strong-globalize@4.0.0 prettier:check script.

npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:

npm ERR!     /home/travis/.npm/_logs/2020-01-22T02_04_36_686Z-debug.log

npm ERR! code ELIFECYCLE

npm ERR! errno 1

npm ERR! strong-globalize@4.0.0 lint: `npm run prettier:check && npm run tslint`

npm ERR! Exit status 1

npm ERR! 

npm ERR! Failed at the strong-globalize@4.0.0 lint script.

npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:

npm ERR!     /home/travis/.npm/_logs/2020-01-22T02_04_36_700Z-debug.log

npm ERR! Test failed.  See above for more details.

The command "npm test" exited with 1.
cache.2

store build cache

Will approve it after the error get fixed.

@jannyHou

This comment has been minimized.

Copy link
Contributor

jannyHou commented Jan 25, 2020

@codechennerator You can run cmd npm run lint:fix to fix the lint error.

@codechennerator codechennerator force-pushed the codechennerator:nc/accept-language branch from 7f369ea to 6a475d3 Jan 27, 2020
Copy link
Contributor

jannyHou left a comment

👍 LGTM thank you @codechennerator !

@hacksparrow

This comment has been minimized.

Copy link
Member

hacksparrow commented Jan 27, 2020

The PR has no description or context. It is not obvious from the changes what the intent of the PR is, and there are no corresponding tests showing the conditions which necessitated the changes.

Please include test(s) before landing.

Copy link
Member

hacksparrow left a comment

Please add tests demonstrating the issues fixed by these code changes.

@emonddr

This comment has been minimized.

Copy link

emonddr commented Jan 27, 2020

The PR has no description or context. It is not obvious from the changes what the intent of the PR is, and there are no corresponding tests showing the conditions which necessitated the changes.

Please include test(s) before landing.

I agree with @hacksparrow .

@codechennerator , please provide a complete description of the problem, and how your code change fixes this problem. Also provide the necessary mocha tests.
Thank you.

@@ -11,7 +11,7 @@ var test = require('tap').test;

SG.SetRootDir(__dirname);
SG.SetDefaultLanguage();
SG.SetAppLanguages(['en', 'zh-cn', 'zh-Hans']);
SG.SetAppLanguages(['en', 'zh-Hans']);

This comment has been minimized.

Copy link
@emonddr

emonddr Jan 27, 2020

How does this solve the original problem(s) you describe?:

g.http() and g.setLanguage() should support mapping zh-cn to zh-Hans and zh-tw to zh_Hant #150

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jan 27, 2020

Contributor

@emonddr I updated the story description and to answer your question, see this:

The http function doesn't map the alias properly when the alias language is not in the appLanguages.

The original test passes because 'zh-cn' is included in the app languages, which will not happen in a real scenario.
The updated test is correct.

This comment has been minimized.

Copy link
@codechennerator

codechennerator Jan 27, 2020

Author Contributor

Yes, to further expand on that, the accept-language module used here right below my changes, does not equivocate zh-cn to zh-Hans. Thus if your AppLanguages listed zh-Hans, en and the request had an accept-language header of zh-cn, the module will pass back en.

Strong-globalize, however, DOES equivocate. (according to implementation of CLDR: "This means that zh ~ zh-CN ~ zh-Hans ~ zh-Hans-CN, and that zh-Hant ~ zh-TW ~ zh-Hant-TW.") And my understanding is that strong-globalize expects its users to set up their intl folder structure to use zh-Hans if they want to support zh-cn.

(Out of the box languages: out-of-box languages - 31: de, en, es, fr, it, ja, ko, pt, ru, zh-Hans, zh-Hant', ar', 'bn', 'cs', 'el', 'fi', 'hi', 'id', 'lt', 'nb', 'nl', 'pl', 'ro', 'sl', 'sv', 'ta', 'te', 'th', 'tr', 'uk', 'vi')

As a result, if you try to set a folder name to zh-cn, strong-globalize will not direct to the right translation. (This might be another feature in itself, but not the one I'm describing) If you did try to add it using the strong-globalize-util though, you would also find that it is not an option as the zh options listed in /cldr-data/main/ are zh, zh-Hans, zh-Hans-HK, zh-Hans-MO, zh-Hans-SG, zh-Hant, zh-Hant-HK, zh-Hant-MO

Thus, this explains why I had to getLangAlias BEFORE we passed it to accept-language and also why I changed the test to not include zh-cn in the AppLanguages.

This comment has been minimized.

Copy link
@emonddr

emonddr Jan 28, 2020

@codechennerator , thx for the clarification :)

@jannyHou

This comment has been minimized.

Copy link
Contributor

jannyHou commented Jan 27, 2020

@hacksparrow

Please add tests demonstrating the issues fixed by these code changes.

He updated the test, see https://github.com/strongloop/strong-globalize/pull/153/files#diff-15748b658c2edfce1211fb1b911af498R14

I can add the description: see comment #150 (comment)

Copied from comment

I think our tests missed the scenario I'm running into now: The http function doesn't map the alias properly when the alias language is not in the appLanguages.

For example: Say my acceptedLanguages (which is determined by my folder structure) is cs, en, zh-Hans, zh-Hant.

g.http will not map properly to zh-Hans because zh-cn is not in the appLanguages. Instead, the acceptLanguage function will return the first language in the appLanguages list. (In this case, cs) When getLangAlias is called it simply matches cs with cs, but by then the language is wrong.

This was not caught in the test because in the test we included zh-cn as part of the appLanguages. However alias conversion should work if appLanguages = en, zh-Hans. Otherwise, the intl folder structure would be:

intl
├── en
├── zh-cn
├── zh-Hans

which is incorrect if zh-Hans is to equivocate to zh-cn.

@@ -11,7 +11,7 @@ var test = require('tap').test;

SG.SetRootDir(__dirname);
SG.SetDefaultLanguage();
SG.SetAppLanguages(['en', 'zh-cn', 'zh-Hans']);
SG.SetAppLanguages(['en', 'zh-Hans']);

This comment has been minimized.

Copy link
@emonddr

emonddr Jan 28, 2020

@codechennerator , thx for the clarification :)

@jannyHou jannyHou merged commit 0e6a799 into strongloop:master Jan 28, 2020
4 checks passed
4 checks passed
Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@codechennerator codechennerator deleted the codechennerator:nc/accept-language branch Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.