search

elixir-cldr / cldr_territories

Territory formatting functions for the Common Locale Data Repository (CLDR) package https://github.com/elixir-cldr/cldr
Other
23 stars 11 forks source link

Refactor backend translations #12

Closed kipcole9 closed 3 years ago

kipcole9 commented 3 years ago
  1. Refactor from_territory_code/4 and from_subdivision_code/3 to re-used already generated backend data
  2. Refactor translate_territory/3 to add a style parameter and to reuse already generated backend data including new inverted_territories/1 mappings.
  3. Refactor from_subdivision_code/3 to use already generated backend data
  4. Refactor translate_subdivision/3 to reuse already generated backend data including new inverted_territories/1 mappings.
  5. When translating territories or subdivisions, normalize the lookup key during inverted mapping creation and during lookup to maximise the change of matching. See Cldr.Territory.normalize_name/1.

There are 9 tests that do not pass and they fit into the following categories:

  1. When translating a territory name a style parameter is required in this code. It defaults to :standard. Some tests make other assumptions.

  2. This PR returns an error when translating a subdivision that has no translation. The current tests assume that {:ok, nil} is returned. I changed the behaviour to match translate_territory/4`.

The code does not at this time use the locale fallback chain when a locale doesn't have territory or division data. This will be added in a separate PR.

CLDR data, but design, doesn't duplicate data when its the same across locale inheritance hence for this data, using the fallback chain is required.

Schultzer commented 3 years ago

Thanks @kipcole9, this looks great!

When translating a territory name a style parameter is required in this code. It defaults to :standard. Some tests make other assumptions

I think, it's fine to just change/remove tests that assumes the wrong behavior.

This PR returns an error when translating a subdivision that has no translation. The current tests assume that {:ok, nil} is returned. I changed the behaviour to match translate_territory/4`

Yeah, it make sense to keep the same behavior, I'm just wondering if it could be improved with a better error message. With that and the fallback chain in mind, I suspect your intention is to only return an error message, if we can't resolve the translation right?

If you don't mind then I can push a commit that fixes the tests and merge this in.

Let me know what you think.

kipcole9 commented 3 years ago

I think, it's fine to just change/remove tests that assumes the wrong behavior.

Happy to do so if thats ok with you.

I suspect your intention is to only return an error message, if we can't resolve the translation right?

Yes, that was my thinking. I think thats consistent with the other functions in the library unless I missed something.

kipcole9 commented 3 years ago

Have updated and pushed so I think good for a merge if you're ok with it (CI is processing). Will look at fallback processing as the next step after this part is good to go.