i18n of SMS about rapidsms HOT 16 CLOSED

Proposed patch in rowenaluk fork, rapidsms/master branch, commit 26f14329, 5b0635, and 689986

from rapidsms.

• The change to rapidsms/webui/settings.py appears to introduce a dependency on a contrib/ directory. What happens if this directory is not present?
• I would like to see the block in manager.py starting with 'if "i18n" in conf' broken out into a helper function. Please see the latest version of this file in trunk under import_local_settings() for an example of how we can do this. Once this function is broken out, I would like to see a docstring and tests added for it to tests/test_manager.py. (you will need to pull from trunk to get this file)
• The tests added to lib/rapidsms/tests/scripted.py appear to introduce a somewhat bizarre looking dependency on Eclipse? Please fix.

Other than those concerns, I think this patch is really well thought out and seems to address the problem well.

from rapidsms.

Correction, I have another couple of concerns:

• I would prefer not to add a top level directory utilities/ without discussion on the mailing list. Perhaps the work being done here can be added as an if __name__ == "__main__" block at the bottom of i18n.py, so that the lib can be used as the checker script itself?
• Please add docstrings to i18n.py.
• Please add test_i18n.py to tests/runtests.py

from rapidsms.

Final concern, what happens if rapidsms route is run with no i18n settings in rapidsms.ini?

from rapidsms.

The change to rapidsms/webui/settings.py appears to introduce a dependency on a contrib/ directory. What happens if this directory is not present?

The code I submitted raised an error asking for the appropriate language file to be generated – now that I’ve thought about it more, I’ve changed it to do what django and python do – default to nothing. SO:

• If this directory is not present and [i18n] is not in rapidsms.ini, everything runs exactly as it used to.
• If [i18n] is in rapidsms.ini, but no other settings are specified, then the default is assumed to be english for both web and sms. All marked strings such as _(“string”) will return “string”.
• If [i18n] is in rapidsms.ini and languages are specified, both web and sms will try

to find the appropriate translation file and if not default to english for that translation

I would like to see the block in manager.py starting with 'if "i18n" in conf' broken out into a helper function. Please see the latest version of this file in trunk under import_local_settings() for an example of how we can do this. Once this function is broken out, I would like to see a docstring and tests added for it to tests/test_manager.py. (you will need to pull from trunk to get this file)

Great idea. Done!
The tests added to lib/rapidsms/tests/scripted.py appear to introduce a somewhat bizarre looking dependency on Eclipse? Please fix.
That line doesn’t introduce a dependency – rather, it improves error reporting for folks working within constrained consoled trying to debug multibyte character sets. Basically, everything that used to work continues to work. However, strange cases of where running something like::

(“string”) % value

Fail because “string” and ‘value’ are strange character types will now report in a somewhat cohesive manner (instead of failing on an ‘unprintable AssertionError object’)
I would prefer not to add a top level directory utilities/ without discussion on the mailing list. Perhaps the work being done here can be added as an if name == "main" block at the bottom of i18n.py, so that the lib can be used as the checker script itself?
No strong feelings on this manner. I put the script in utilities because that folder existed already when I pulled from rapidsms/HEAD. Is there some place that we generally put utility scripts – in contrib somewhere?
Please add docstrings to i18n.py.
Done.
Please add test_i18n.py to tests/runtests.py
Done

Changes here: af3193b

from rapidsms.

1. re the explanation of the i18n behavior, please add that to the docs?

2. re the utility script, if you wouldn't mind just adding that behavior to the library file itself, we can dodge that particular question for now, and refer it to the mailing list.

With these changes, I will approve and merge these commits. Thanks!

from rapidsms.

1. <a href="http://github.com/rapidsms/rapidsms-documentation/commit/894349269015f83ddc9ef065cc9247837183ce2e>89434

2. I was merging it into the library file, when I realized that this is only going to create a dependency on pygsm - because python has codecs built-in for dozens of other character encodings (ascii, utf-8, etc.) but it doesn't have one built-in for gsm - jeff wrote that custom fro pygsm. So I can't put it in the lib file. I'm fine with not including the utility in this release, though - it's not core functionality anyways.

from rapidsms.

is this issue finished? if so, please close it

from rapidsms.

Heh. I also seem unable to close issues. =b

from rapidsms.

I consider this a bug in github.

from rapidsms.

This was never applied. Ro has repackaged the changes here: which are now being merged in. http://github.com/rluk/rapidsms/commit/d8494beb43006151da360726d3f363a2eeb738d1

from rapidsms.

Merged those in commit 2f6bec6

from rapidsms.

A second patch proposal for tests has been submitted by Rowena as well: http://github.com/rluk/rapidsms/commit/c9d2ea986bbe4ad6194f2097cf4774f4ee083958

from rapidsms.

I took a look at this patch and think that it would be good if there were unit tests for the functionality (primarily sending in poorly encoded strings and checking the various error conditions). However, since the patch only touches test code I think it can go in as-is provided all tests currently pass. I think we should allow this commit through and create a second issue that we need unicode tests in the core.

from rapidsms.

Exactly the same commit as c9d2ea9, only with comments improved.

17d7828

Also, fixed rapidsms lib unit tests:

be083475

And now get sms translators working properly for rapidsms regular unit tests:

2532bbb

from rapidsms.

Ok, fixed and pushed.

from rapidsms.