search

strongloop / strong-globalize

strong-globalize is built on Unicode CLDR and jquery/globalize and implements automatic extraction of strings from JS source code and HTML templates, lint the string resource, machine-translate them in seconds. In runtime, it loads locale and string resource into memory and provides a hook to persistent logging.
Other
25 stars 16 forks source link

Fix language alias for http function #153

Closed codechennerator closed 4 years ago

codechennerator commented 4 years ago

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 commented 4 years ago

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

jannyHou commented 4 years ago

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

hacksparrow commented 4 years ago

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.

emonddr commented 4 years ago

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.

jannyHou commented 4 years ago

@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 https://github.com/strongloop/strong-globalize/issues/150#issuecomment-576942219

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.