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

Territory languages #315

Merged
merged 5 commits into from Jan 11, 2016
Merged

Territory languages #315

merged 5 commits into from Jan 11, 2016

Conversation

akx
Copy link
Member

@akx akx commented Jan 3, 2016

This PR is a rebase of and supersedes #122 and #123.

@akx akx added this to the Babel 2.3/3.0 milestone Jan 3, 2016
@codecov-io
Copy link

Current coverage is 88.34%

Merging #315 into master will increase coverage by +0.05% as of 6d21d4e

@@            master    #315   diff @@
======================================
  Files           22      23     +1
  Stmts         3597    3612    +15
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3176    3191    +15
  Partial          0       0       
  Missed         421     421       

Review entire Coverage Diff as of 6d21d4e

Powered by Codecov. Updated on successful CI builds.

@etanol
Copy link
Contributor

etanol commented Jan 7, 2016

Even though the population percents may be informative, I have the feeling that it will become outdated very quickly. Therefore, unless we can keep up with the bi-yearly CLDR release schedule, we may be left behind again (like when we supported CLDR 23) and, then, such data will not be very useful.

On the other hand, the supported languages by territory will hopefully change in a less dramatic way.

One solution could be to add a big fat disclaimer note regarding the possible lack of accuracy for the population percent information.

@akx
Copy link
Member Author

akx commented Jan 7, 2016

@etanol: I'll make an issue of adding that big fat disclaimer.

Disclaimer added in cbd98d4.

If you think this is good to merge otherwise, go ahead and do it :)

Available in the global data onder the 'territory_language' key.
for language, info in get_global("territory_languages").get(territory, {}).items():
if info["official_status"] in allowed_stati:
pairs.append((info["population_percent"], language))
pairs.sort(reverse=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be inclined to write:

languages = get_global("territory_languages").get(territory, {})
pairs = [(info['population_percent'], language)
             for language, info in langs.iteritems()
                 if info['official_status'] in allowed_stati]
pairs.sort(reverse=True)

It should be a little faster and a bit more Pythonic to make more extensive use of comprehensions.

@akx
Copy link
Member Author

akx commented Jan 11, 2016

Switched to an admittedly more pythonic and more or less just as readable list comprehension as suggested by @etanol.

Since patch coverage is 100% and overall coverage is increasing, I'll just go ahead and merge this :)

akx added a commit that referenced this pull request Jan 11, 2016
@akx akx merged commit a4cd0e3 into python-babel:master Jan 11, 2016
@akx akx deleted the territory-languages branch January 11, 2016 09:15
@pyup-bot pyup-bot mentioned this pull request Apr 11, 2017
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

6 participants