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

Moved i18n data to core/Intl #7996

Merged
merged 7 commits into from May 28, 2015
Merged

Moved i18n data to core/Intl #7996

merged 7 commits into from May 28, 2015

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented May 25, 2015

Instead of moving the i18n-data to a separate repository, they are now located in core/Intl.

I've generated those files out of the old Piwik data and used data from unicode-cldr/cldr-localenames-full to improve them.

To make it easy to update those data, I've added a new console command intl:generate, that allows updating the files using the current master of unicode-cldr/cldr-localenames-full.

@sgiehl sgiehl added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label May 25, 2015
@mattab
Copy link
Member

mattab commented May 27, 2015

Feedback:

  • Nice idea to reuse unicode-cldr I didn't know this existed and it looks useful! it's a really great added value to translators that they don't have to translate 250+ country names! 👍
  • Maybe the new command could be moved in translations:generate-intl-data instead of intl:generate
  • Looks good to me

@sgiehl
Copy link
Member Author

sgiehl commented May 27, 2015

I'll rename the console command later. Guess we should also be able to use other parts of unicode-cldr. It also includes names of months & weekdays as well as time formats. I'll have a look at that within the next days

@sgiehl sgiehl added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels May 27, 2015
@sgiehl
Copy link
Member Author

sgiehl commented May 27, 2015

Should be finished so far. I'll add some more intl data like continents, day and month names, etc in another PR to keep them small.

@mnapoli
Copy link
Contributor

mnapoli commented May 28, 2015

Funnily enough a UI test "broke" because Russian Federation became Russia :) (it's fine)

AllTests fails because of what seems to be no disk space left :/ Is it one of the random failures or something new?

Except the AllTests looks good to merge

@mnapoli
Copy link
Contributor

mnapoli commented May 28, 2015

By they way there is no way we download directly the original package (unicode-cldr/cldr-localenames-full) using a package manager? That would avoid having to update the translations if this could be handled by another system like Composer for example. And it would avoid to commit several megabytes in the repo.

They have npm or Bower integration, but I'm not sure it's a good idea for us (PHP project) to use this. But it seems there are other packages on Packagist that contain the same data.

The thing is: does it make sense to commit such data in Piwik's repository. The idea with core/Intl/Resources was to make it easier to eventually replace it with a 3rd party lib, because why would we maintain that ourselves?

@sgiehl
Copy link
Member Author

sgiehl commented May 28, 2015

For sure we could use the full dataset of unicode-cldr/cldr-localenames-full. But it contains a lot of languages and Piwik would only use a small part of it.
That would mean we include a big bunch of data without any use.
Additionally unicode-cldr/cldr-localenames-full only contains the language and country names, for month and day names we would also need unicode-cldr/cldr-dates-full

Small Note: We would only need to run the console command to update the intl data in two cases:

  • a new language is added to piwik (which happens not so often)
  • new unicode-cldr/cldr-localenames-full release (which happens twice a year, according to their release plan)

@mnapoli
Copy link
Contributor

mnapoli commented May 28, 2015

Ah OK you only fetch what we need, then 👍

sgiehl pushed a commit that referenced this pull request May 28, 2015
Moved i18n data to core/Intl
@sgiehl sgiehl merged commit 9438ac1 into master May 28, 2015
@sgiehl sgiehl deleted the i18n-data branch May 28, 2015 15:34
@sgiehl sgiehl removed the Needs Review PRs that need a code review label May 28, 2015
@sgiehl sgiehl added this to the 2.14.0 milestone May 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants