Add support for an alternate localedir

[sayamindu]

This is an implementation for ​http://wiki.sugarlabs.org/go/Features/Enhanced_Gettext without breaking/adding to the API.

The idea is pretty simple - before doing a bindtextdomain(), the candidate locale directories are scanned, and the one with the latest translations is chosen. (this is due to the fact that an activity may be updated, through which it may get newer translations than what is there in the langpackdir).

It is less invasive than what I originally proposed, and seems to do what it needs to. It would need some testing though, since there may be a few corner cases I might have missed.

The relevant patches are in:

• ​http://git.sugarlabs.org/projects/sugar-toolkit/repos/gettext-enhancements/commits/ef093d395e3b682a4f1cc603b385fecb09298cff
• ​http://git.sugarlabs.org/projects/sugar-toolkit/repos/gettext-enhancements/commits/173a4027019e0adff04534176167ac24a0b303cd

It also needs an extra GConf key (added via the Sugar package): /desktop/sugar/i18n/langpackdir

One problem with the code is that it adds a dependency to python-dateutils (for parsing the date). Python-dateutils is however, packaged for most distros. I could not figure out how to parse ISO 8601 formatted strings with the time.strptime().

[erikos]

Thanks for the detailed description of your review request.

Two first comments:
from sugar.activity import activityhandle, i18n

• please use two lines

Please use the docstring format like the one in sugar/datastore/datastore.py (with arguments and return values).

A deeper review will follow tomorrow.

[sayamindu]

Replying to erikos:

Thanks for the detailed description of your review request.

Two first comments:
from sugar.activity import activityhandle, i18n

• please use two lines

Please use the docstring format like the one in sugar/datastore/datastore.py (with arguments and return values).

Done :-).

[erikos]

This is the path that gets generated by the script:

/home/erikos/Activities/Memorize.activity/locale/de_DE/LC_MESSAGES/org.laptop.Memorize.mo

However on my system the path is:

/home/erikos/Activities/Memorize.activity/locale/de/LC_MESSAGES/org.laptop.Memorize.mo

This should be an 'and'

if packdir is not None or packdir is not '': 

Smaller ones:

packdir - can you change that member to be packagedir (abbreviations in general)

Watch out for spaces:

ValueError ('Could not find revision date')

I would try to avoid such constructs if possible. At least they are easier to read when splitting them in a few lines.

sorted(candidate_dirs.iteritems(), key=lambda (k, v): (v, k), \
        reverse = True)[0][0]

Unused variables should be with trailing underscore.

version_, numofstrings = _readbin(handle, fmt, 8)

Please run scripts/data/pep8.py and pylint (included in jhbuild). More: ​http://wiki.sugarlabs.org/go/Development_Team/Code_guidelines#Tools

[erikos]

Ping, sayamindu, when do you think we can land this one?

[sayamindu]

Sorry - I was away travelling and my 3G provider let me down by refusing to roam. Give me another day and I'll try to make this into landable stuff

[sayamindu]

I fixed the style and pep8 issues.

Regarding the final return statement, I added a comment about what it does, so there should be less confusion for future hackers.

Regarding the confusion about de_DE and de, I'll need to do a bit more thinking about it, but it is not very critical, and I think we can land the code as it is for now.

[erikos]

About the de_DE and de confusion: I am not sure (testing is a few days ago) it worked for me with this error. Please verify that.

Style: There are still quite some abbreviations in the code. And sometimes you do 'filepath' and then magic_number. Please use the underscore format consistently. As well the argument 'fmt' is not very descriptive.

Comments: The comment for get_locale_path should contain the return value. You might want to add a comment for the module, too. And as this is inner API we should reflect that in some way (not sure yet how to do that).

# Fancy way to sort the dictionary by value

I fear this is redundant information :) The code should be self explanatory if possible. For example you could assign the outcome of

sorted(candidate_dirs.iteritems(), key=lambda (k, v): (v, k), reverse=True)

to variables and return that. In addition with the comment in the docstring this would already make it a lot clearer.

Thanks!

[erikos]

Btw, please attach a new patch next time. This makes it easier for the reviewer :D

[sayamindu]

Consistent variable name underscoring

[sayamindu]

Docstring cleanup

[sayamindu]

Make the return section (dictionary sorting part) make more sense

[sayamindu]

Patches attached.

The problem with de_DE is interesting. For many languages, it is OK when you use de instead of de_DE. However, certain languages are different, for example, pt_BR is different from pt, and bn_IN is different from bn (or bn_BD).

In such a situation, I wonder what the best approach would be. I am trying out a slightly enhanced weighing scheme which may work, but I need a few more days to properly test this out.

[sayamindu]

Unified version of the previously attached patches, as discussed on IRC

[sayamindu]

Merged with mainline. Thanks for the thorough review. Closing this ticket.

[dnarvaez]

Milestone 0.88 deleted