Issue 7042 - [XTM Integration] Solve UnicodeDecodeErrors occuring

Noissue - [XTM Integration] Added support for unicode characters in translation strings

[Tudor Avram, 2018-10-10 13:55:07]

Hi Vasily and Winsley,

Here is a simple fix that should handle unicode strings in pages that we want to
upload for translation. I also added a test for that.

Thanks, 
Tudor.

[Vasily Kuznetsov, 2018-10-10 14:01:26]

Hi Tudor!

See my comments below.

Cheers,
Vasily

https://codereview.adblockplus.org/29906560/diff/29906561/cms/translations/xt...
File cms/translations/xtm/utils.py (right):

https://codereview.adblockplus.org/29906560/diff/29906561/cms/translations/xt...
cms/translations/xtm/utils.py:15: from __future__ import unicode_literals
Nit: empty string after the top comment

https://codereview.adblockplus.org/29906560/diff/29906561/tests/test_xtm_tran...
File tests/test_xtm_translations_utils.py (right):

https://codereview.adblockplus.org/29906560/diff/29906561/tests/test_xtm_tran...
tests/test_xtm_translations_utils.py:15: from __future__ import unicode_literals
Nit: empty line after the top comment

https://codereview.adblockplus.org/29906560/diff/29906561/tests/xtm_conftest.py
File tests/xtm_conftest.py (right):

https://codereview.adblockplus.org/29906560/diff/29906561/tests/xtm_conftest....
tests/xtm_conftest.py:117: @pytest.fixture
I would say that it makes sense to just add this file to the tests site instead.
It's useful to tests that other parts of the CMS can handle non-ascii strings.

[Tudor Avram, 2018-10-11 13:58:28]

Hi Vasily,

Thanks for your comments. I addressed them (see below). 

Tudor.

P.S.: `tests/xtm_conftest.py` is no longer present in the new patch, as it is
exactly the same as master.

https://codereview.adblockplus.org/29906560/diff/29906561/cms/translations/xt...
File cms/translations/xtm/utils.py (right):

https://codereview.adblockplus.org/29906560/diff/29906561/cms/translations/xt...
cms/translations/xtm/utils.py:15: from __future__ import unicode_literals
On 2018/10/10 14:01:26, Vasily Kuznetsov wrote:
> Nit: empty string after the top comment

Done.

https://codereview.adblockplus.org/29906560/diff/29906561/tests/test_xtm_tran...
File tests/test_xtm_translations_utils.py (right):

https://codereview.adblockplus.org/29906560/diff/29906561/tests/test_xtm_tran...
tests/test_xtm_translations_utils.py:15: from __future__ import unicode_literals
On 2018/10/10 14:01:26, Vasily Kuznetsov wrote:
> Nit: empty line after the top comment

Done.

https://codereview.adblockplus.org/29906560/diff/29906561/tests/xtm_conftest.py
File tests/xtm_conftest.py (right):

https://codereview.adblockplus.org/29906560/diff/29906561/tests/xtm_conftest....
tests/xtm_conftest.py:117: @pytest.fixture
On 2018/10/10 14:01:26, Vasily Kuznetsov wrote:
> I would say that it makes sense to just add this file to the tests site
instead.
> It's useful to tests that other parts of the CMS can handle non-ascii strings.

Done.

[Vasily Kuznetsov, 2018-10-12 09:39:51]

Hi Tudor,

Looks good except for one docstring that produces an error because of non-ascii
characters (see below).

Cheers,
Vasily

https://codereview.adblockplus.org/29906560/diff/29907555/tests/test_xtm_tran...
File tests/test_xtm_translations_utils.py (right):

https://codereview.adblockplus.org/29906560/diff/29907555/tests/test_xtm_tran...
tests/test_xtm_translations_utils.py:229: Testing for presence of "Ͷ" - \u0376.
Doesn't this line cause an error for you? I get:

.tox/py27/lib/python2.7/site-packages/_pytest/python.py:482: in
_importtestmodule
    mod = self.fspath.pyimport(ensuresyspath=importmode)
.tox/py27/lib/python2.7/site-packages/py/_path/local.py:668: in pyimport
    __import__(modname)
E     File
"/Users/vkuznetsov/Documents/prog/hg.adblockplus.org/cms/tests/test_xtm_translations_utils.py",
line 229
E   SyntaxError: Non-ASCII character '\xcd' in file
/Users/vkuznetsov/Documents/prog/hg.adblockplus.org/cms/tests/test_xtm_translations_utils.py
on line 230, but no encoding declared; see http://python.org/dev/peps/pep-0263/
for details

I think the right approach here would be to remove the "Ͷ" from the docstring
instead of declaring encoding -- it's better to stick to ASCII in source code
unless you really have to.

[Vasily Kuznetsov, 2018-10-12 09:39:51]

Hi Tudor,

Looks good except for one docstring that produces an error because of non-ascii
characters (see below).

Cheers,
Vasily

https://codereview.adblockplus.org/29906560/diff/29907555/tests/test_xtm_tran...
File tests/test_xtm_translations_utils.py (right):

https://codereview.adblockplus.org/29906560/diff/29907555/tests/test_xtm_tran...
tests/test_xtm_translations_utils.py:229: Testing for presence of "Ͷ" - \u0376.
Doesn't this line cause an error for you? I get:

.tox/py27/lib/python2.7/site-packages/_pytest/python.py:482: in
_importtestmodule
    mod = self.fspath.pyimport(ensuresyspath=importmode)
.tox/py27/lib/python2.7/site-packages/py/_path/local.py:668: in pyimport
    __import__(modname)
E     File
"/Users/vkuznetsov/Documents/prog/hg.adblockplus.org/cms/tests/test_xtm_translations_utils.py",
line 229
E   SyntaxError: Non-ASCII character '\xcd' in file
/Users/vkuznetsov/Documents/prog/hg.adblockplus.org/cms/tests/test_xtm_translations_utils.py
on line 230, but no encoding declared; see http://python.org/dev/peps/pep-0263/
for details

I think the right approach here would be to remove the "Ͷ" from the docstring
instead of declaring encoding -- it's better to stick to ASCII in source code
unless you really have to.

[Vasily Kuznetsov, 2018-10-12 09:40:46]

Looks like Rietveld doesn't have protection against double click on a button, he
he :D

[Tudor Avram, 2018-10-12 10:37:02]

Hi Vasily, 

Thanks for your feedback, removed the non-ASCII characters now. 

Thanks, 
Tudor.

https://codereview.adblockplus.org/29906560/diff/29907555/tests/test_xtm_tran...
File tests/test_xtm_translations_utils.py (right):

https://codereview.adblockplus.org/29906560/diff/29907555/tests/test_xtm_tran...
tests/test_xtm_translations_utils.py:229: Testing for presence of "Ͷ" - \u0376.
On 2018/10/12 09:39:51, Vasily Kuznetsov wrote:
> Doesn't this line cause an error for you? I get:
> 
> .tox/py27/lib/python2.7/site-packages/_pytest/python.py:482: in
> _importtestmodule
>     mod = self.fspath.pyimport(ensuresyspath=importmode)
> .tox/py27/lib/python2.7/site-packages/py/_path/local.py:668: in pyimport
>     __import__(modname)
> E     File
>
"/Users/vkuznetsov/Documents/prog/hg.adblockplus.org/cms/tests/test_xtm_translations_utils.py",
> line 229
> E   SyntaxError: Non-ASCII character '\xcd' in file
>
/Users/vkuznetsov/Documents/prog/hg.adblockplus.org/cms/tests/test_xtm_translations_utils.py
> on line 230, but no encoding declared; see
http://python.org/dev/peps/pep-0263/
> for details
> 
> I think the right approach here would be to remove the "Ͷ" from the docstring
> instead of declaring encoding -- it's better to stick to ASCII in source code
> unless you really have to.

Huh, ok. It didn't give me an error, but well,  I guess it makes sense to have
ASCII only characters in the codebase. Changing it now.

[Vasily Kuznetsov, 2018-10-16 12:22:12]

On 2018/10/12 10:37:02, Tudor Avram wrote:
> Hi Vasily, 
> 
> Thanks for your feedback, removed the non-ASCII characters now. 
> 
> Thanks, 
> Tudor.
> 
>
https://codereview.adblockplus.org/29906560/diff/29907555/tests/test_xtm_tran...
> File tests/test_xtm_translations_utils.py (right):
> 
>
https://codereview.adblockplus.org/29906560/diff/29907555/tests/test_xtm_tran...
> tests/test_xtm_translations_utils.py:229: Testing for presence of "Ͷ" -
\u0376.
> On 2018/10/12 09:39:51, Vasily Kuznetsov wrote:
> > Doesn't this line cause an error for you? I get:
> > 
> > .tox/py27/lib/python2.7/site-packages/_pytest/python.py:482: in
> > _importtestmodule
> >     mod = self.fspath.pyimport(ensuresyspath=importmode)
> > .tox/py27/lib/python2.7/site-packages/py/_path/local.py:668: in pyimport
> >     __import__(modname)
> > E     File
> >
>
"/Users/vkuznetsov/Documents/prog/hg.adblockplus.org/cms/tests/test_xtm_translations_utils.py",
> > line 229
> > E   SyntaxError: Non-ASCII character '\xcd' in file
> >
>
/Users/vkuznetsov/Documents/prog/hg.adblockplus.org/cms/tests/test_xtm_translations_utils.py
> > on line 230, but no encoding declared; see
> http://python.org/dev/peps/pep-0263/
> > for details
> > 
> > I think the right approach here would be to remove the "Ͷ" from the
docstring
> > instead of declaring encoding -- it's better to stick to ASCII in source
code
> > unless you really have to.
> 
> Huh, ok. It didn't give me an error, but well,  I guess it makes sense to have
> ASCII only characters in the codebase. Changing it now.

LGTM