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 incorrectly named zh-hans and zh-hant locale path #4103

Merged
merged 2 commits into from May 9, 2016

Conversation

Hongxia
Copy link
Contributor

@Hongxia Hongxia commented May 7, 2016

Django discovers translations for simplified Chinese (zh-hans) and traditional Chinese (zh-hant) by looking up the locale name in rest_framework/locale/ directory, meaning it's looking for rest_framework/locale/zh_Hans/ and rest_framework/locale/zh_Hant/.

@codecov-io
Copy link

codecov-io commented May 7, 2016

Current coverage is 91.39%

Merging #4103 into master will not change coverage

@@             master      #4103   diff @@
==========================================
  Files            51         51          
  Lines          5473       5473          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           5002       5002          
  Misses          471        471          
  Partials          0          0          

Powered by Codecov. Last updated by dbbf79b...f912fae

@xordoquy
Copy link
Collaborator

xordoquy commented May 7, 2016

This will be canceled by the next transfex update.
There was a merged PR to fix this but it seems something didn't work.

@xordoquy xordoquy closed this May 7, 2016
@Hongxia
Copy link
Contributor Author

Hongxia commented May 8, 2016

@xordoquy do you expect the next transfex update to fix zh-hans and zh-hant locale path? The current locale directories are definitely incorrect (see the linked Django documentation in the PR description) - Django unable to locale the django.mo binaries.

This fix seems to work perfectly for us so far (we forked DRF 3.3.3). Would you be more specific about what failed when a PR was merged to address it?

@xordoquy
Copy link
Collaborator

xordoquy commented May 8, 2016

To be honest, I have no idea how translation work. The PR was supposed to fix the translation path for zh-hans / zh-hant but I have no idea why this didn't work.

@xordoquy
Copy link
Collaborator

xordoquy commented May 8, 2016

PR is #3739

@Hongxia
Copy link
Contributor Author

Hongxia commented May 9, 2016

I'm not familiar with how DRF work with transifex - it's hard to dig around without transifex account credentials.

But after spending sometime reading transifex documentation and this change from #3739, it seems like the when you run tx pull -a, transifex will pull the simplified Chinese translation file (transifex remote language code zh-Hans) into rest_framework/locale/<lang>/LC_MESSAGES/django.po where lang is zh_Hans.

In other words, I'm guessing that when you pull updated translations from transifex, it actually won't counterfeit the changes in this PR.

@xordoquy xordoquy reopened this May 9, 2016
@Hongxia
Copy link
Contributor Author

Hongxia commented May 9, 2016

Never mind, just realized that I could run tx pull with my own test transifex account, and I was able to prove my earlier hypothesis.

With zh-Hans/t renamed to zh_Hans/t, tx pull -a correctly updates the corrected locale path thanks to lang_map = sr@latin:sr_Latn, zh-Hans:zh_Hans, zh-Hant:zh_Hant in .tx/config. In addition, if you delete the zh-Hans/t paths before running tx pull -a, transifex client actually will create the correct locale path for you, i.e. zh_Hans and zh_Hant.

@xordoquy
Copy link
Collaborator

xordoquy commented May 9, 2016

Alright, I now have an idea why #3739 didn't work.
I just removed the zh-Han* locale dirs, did a tx update -a and somehow zh-Han* became zh_Han*
However, as long as I do have a zh-Han* directory, translations will go into that one.
This change goes with #3739 to fully fix the issue.

Thanks for taking the time to investigate !

@xordoquy xordoquy merged commit 5ac9685 into encode:master May 9, 2016
2 of 3 checks passed
@xordoquy xordoquy modified the milestones: 3.4.0 Release, 3.3.4 Release May 9, 2016
@Hongxia
Copy link
Contributor Author

Hongxia commented May 9, 2016

You're welcome! Glad I could help :)

@xordoquy
Copy link
Collaborator

xordoquy commented May 9, 2016

Seems we did the same tests in parallel :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants