Opened 4 years ago

Closed 4 years ago

#3076 closed change (fixed)

[CMS] Map locale names to match what Crowdin expects during synchronisation

Reported by: kzar Assignee: kzar
Priority: P3 Milestone:
Module: Sitescripts Keywords: cms
Cc: sebastian, saroyanm Blocked By:
Blocking: #3049 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29328085/

Description (last modified by kzar)

Background

In issue #3049 it was reported the wrong language (English) was being served for canonical URLs on adblockbrowser.org when the Accept-Language header contained a hyphen. This regression was caused when locale directories were renamed to match the names Crowdin expected.

Now that we have to undo that change we will again be unable to easily synchronise translations with Crowdin. We will therefore need a way to map our locale names to the Crowdin equivalents.

What to change

Update the Crowdin synchronisation script to map our locale names to the Crowdin equivalents. To turn a local locale name into the Crowdin version the following steps should be taken:

1.) If the name contains an underscore, we can just replace it with a hyphen.
2.) Otherwise check if the name is supported by Crowdin using the supported languages API. If already supported, we're done without modifications.
3.) Otherwise take an upper-case version of the locale name and append it after a hyphen. For example "xx" becomes "xx-XX". If that's supported we're now done.
4.) (If at step 1 or 3 we realise the locale is unsupported, the _unmodified_ locale name should be used when issuing a warning to the user.)

Example mappings:

es = es-ES
de = de
pt = pt-PT
zh_TW = zh-TW

Change History (8)

comment:1 Changed 4 years ago by kzar

  • Blocking 3049 added

comment:2 follow-up: Changed 4 years ago by sebastian

The background section is inaccurate. #3049 isn't a regression. This issue existed ever since for adblockbrowser.org. We used CrowdIn locales there from the beginning.

Moreover, I don't think this mapping need to be configurable but should rather be hard-coded to enforce consistency and single point of failure. As far as I can tell it can even be expressed with a simple function:

def convert_from_crowdin_locale(locale)
  parts = locale.split('-')
  if len(parts) == 1 or parts[0] == parts[1].lower():
    return parts[0]
  return '_'.join(parts[:2])


comment:3 in reply to: ↑ 2 Changed 4 years ago by kzar

Replying to sebastian:

The background section is inaccurate. #3049 isn't a regression. This issue existed ever since for adblockbrowser.org. We used CrowdIn locales there from the beginning.

Well I thought so too but then I noticed the change was made in d19bfbe47ed8. (However that commit was pushed before the site went live I suppose.)

Moreover, I don't think this mapping need to be configurable but should rather be hard-coded to enforce consistency and single point of failure. As far as I can tell it can even be expressed with a simple function:

def convert_from_crowdin_locale(locale)
  parts = locale.split('-')
  if len(parts) == 1 or parts[0] == parts[1].lower():
    return parts[0]
  return '_'.join(parts[:2])


Perhaps we can do this without manually configuring the mappings, but in that case we will also need a function to convert from the local locale names to the Crowdin ones. How can that function know that "es" should be "es-ES" but "de" should remain as "de"? (I certainly agree that making this automatic is desirable.)

comment:4 Changed 4 years ago by sebastian

My point was that this doesn't need to be configurable. But should rather be a hard-coded mapping, if not a function. Of course I'd prefer to use the algorithm above rather than hard-coding the mapping. But sure, it doesn't work the other way around. But if we know the list of CrowdIn locales at runtime, we can easily build a reverse mapping using the same function. Or we can try different variants at runtime, so if there is no es on CrowdIn, fallback to es-ES.

Last edited 4 years ago by sebastian (previous) (diff)

comment:5 Changed 4 years ago by kzar

  • Description modified (diff)
  • Summary changed from [CMS] Add locale name mapping functionality to the CMS to [CMS] Map locale names to match what Crowdin expects during synchronisation

Whoops, of course. I was thinking about this last night and realised it was actually pretty simple to implement with the mapping working automatically like you suggested. I've updated the ticket.

comment:6 Changed 4 years ago by sebastian

  • Priority changed from Unknown to P3
  • Ready set

comment:7 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:8 Changed 4 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.