Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(301)

Issue 29906560: Issue 7042 - [XTM Integration] Solve UnicodeDecodeErrors occuring (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 2 days ago by Tudor Avram
Modified:
2 days, 9 hours ago
Reviewers:
wspee, Vasily Kuznetsov
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressed comments from Patch set #1 #

Total comments: 2

Patch Set 3 : Addressed comments from Patch Set #2 #

Patch Set 4 : Fixed flake8 docstring error #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -0 lines) Patch
M cms/translations/xtm/utils.py View 1 1 chunk +2 lines, -0 lines 0 comments Download
M tests/expected_output/en/sitemap View 1 1 chunk +1 line, -0 lines 0 comments Download
A tests/test_site/pages/translate-unicode.md View 1 1 chunk +1 line, -0 lines 0 comments Download
M tests/test_xtm_translations_utils.py View 1 2 3 2 chunks +10 lines, -0 lines 0 comments Download

Messages

Total messages: 8
Tudor Avram
Hi Vasily and Winsley, Here is a simple fix that should handle unicode strings in ...
1 week, 2 days ago (2018-10-10 13:55:07 UTC) #1
Vasily Kuznetsov
Hi Tudor! See my comments below. Cheers, Vasily https://codereview.adblockplus.org/29906560/diff/29906561/cms/translations/xtm/utils.py File cms/translations/xtm/utils.py (right): https://codereview.adblockplus.org/29906560/diff/29906561/cms/translations/xtm/utils.py#newcode15 cms/translations/xtm/utils.py:15: from ...
1 week, 2 days ago (2018-10-10 14:01:26 UTC) #2
Tudor Avram
Hi Vasily, Thanks for your comments. I addressed them (see below). Tudor. P.S.: `tests/xtm_conftest.py` is ...
1 week, 1 day ago (2018-10-11 13:58:28 UTC) #3
Vasily Kuznetsov
Hi Tudor, Looks good except for one docstring that produces an error because of non-ascii ...
1 week ago (2018-10-12 09:39:51 UTC) #4
Vasily Kuznetsov
Hi Tudor, Looks good except for one docstring that produces an error because of non-ascii ...
1 week ago (2018-10-12 09:39:51 UTC) #5
Vasily Kuznetsov
Looks like Rietveld doesn't have protection against double click on a button, he he :D
1 week ago (2018-10-12 09:40:46 UTC) #6
Tudor Avram
Hi Vasily, Thanks for your feedback, removed the non-ASCII characters now. Thanks, Tudor. https://codereview.adblockplus.org/29906560/diff/29907555/tests/test_xtm_translations_utils.py File ...
1 week ago (2018-10-12 10:37:02 UTC) #7
Vasily Kuznetsov
3 days, 11 hours ago (2018-10-16 12:22:12 UTC) #8
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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5