You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by do...@apache.org on 2013/07/02 16:12:44 UTC
svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Author: dongsheng
Date: Tue Jul 2 14:12:44 2013
New Revision: 1498947
URL: http://svn.apache.org/r1498947
Log:
Make sure msgstr is not None before do iteration.
Obvious fix.
* tools/dev/po-merge.py
Since parse_translation maybe return (comments, None, None, None),
we should not do iteration without check msgstr is not None. Otherwise,
we may encounter the following TypeError:
Traceback (most recent call last):
File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
main(sys.argv)
File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
for m in msgstr:
TypeError: 'NoneType' object is not iterable
Modified:
subversion/trunk/tools/dev/po-merge.py
Modified: subversion/trunk/tools/dev/po-merge.py
URL: http://svn.apache.org/viewvc/subversion/trunk/tools/dev/po-merge.py?rev=1498947&r1=1498946&r2=1498947&view=diff
==============================================================================
--- subversion/trunk/tools/dev/po-merge.py (original)
+++ subversion/trunk/tools/dev/po-merge.py Tue Jul 2 14:12:44 2013
@@ -178,9 +178,10 @@ def main(argv):
for i in msgstr:
outfile.write('msgstr[%s] %s\n' % (n, msgstr[n]))
n += 1
- for m in msgstr:
- if m == '""':
- untranslated += 1
+ if msgstr is not None:
+ for m in msgstr:
+ if m == '""':
+ untranslated += 1
for c in comments:
if c.startswith('#,') and 'fuzzy' in c.split(', '):
fuzzy += 1
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Daniel Shahaf <da...@elego.de>.
Dongsheng Song wrote on Tue, Jul 02, 2013 at 23:00:59 +0800:
> 于 2013/7/2 22:30, Daniel Shahaf 写道:
> >dongsheng@apache.org wrote on Tue, Jul 02, 2013 at 14:12:44 -0000:
> >>Author: dongsheng
> >>Date: Tue Jul 2 14:12:44 2013
> >>New Revision: 1498947
> >>
> >>URL: http://svn.apache.org/r1498947
> >>Log:
> >>Make sure msgstr is not None before do iteration.
> >>
> >>Obvious fix.
> >>
> >No, it's not an obvious fix. It's a code change, and one which is not
> >obvious, so it requires review.
> >
> >> Since parse_translation maybe return (comments, None, None, None),
> >When does that happen? Why is that not a bug? Why no translator run
> >into it until today?
> Today, when I merge zh_CN.po from trunk to 1.8.x, I had encounter
> the following error:
>
> $ python ../../../../trunk/tools/dev/po-merge.py <
> ../../../../trunk/subversion/po/zh_CN.po zh_CN.po
> Traceback (most recent call last):
> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
> main(sys.argv)
> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
> for m in msgstr:
> TypeError: 'NoneType' object is not iterable
>
> Then I found in the line 39-40 of po-merge.py return None as msgstr:
>
> if line.strip() == '' or line[:2] == '#~':
> return comments, None, None, None
>
> So we should not do iteration on msgstr without make sure msgstr is
> not None.
>
> This happened because zh_CN.po have msgmerged po comments like this:
>
> #~ msgid "Uncommitted local addition, copy or move%s"
> #~ msgstr "未提交的本地增加，复制或移动 %s"
>
Thanks. That explains why msgstr was None. But I still don't
understand why ignoring the for loop is the correct fix.
> As your judgement, this is not "obvious fix", should I revert this commit ?
It'd be simpler if someone looked at the commit and gave it his +1.
Daniel
>
> >Thanks,
> >
> >Daniel
> >
> >>* tools/dev/po-merge.py
> >> we should not do iteration without check msgstr is not None. Otherwise,
> >> we may encounter the following TypeError:
> >>
> >> Traceback (most recent call last):
> >> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
> >> main(sys.argv)
> >> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
> >> for m in msgstr:
> >> TypeError: 'NoneType' object is not iterable
> >>
> >>Modified:
> >> subversion/trunk/tools/dev/po-merge.py
> >>
> >>Modified: subversion/trunk/tools/dev/po-merge.py
> >>URL: http://svn.apache.org/viewvc/subversion/trunk/tools/dev/po-merge.py?rev=1498947&r1=1498946&r2=1498947&view=diff
> >>==============================================================================
> >>--- subversion/trunk/tools/dev/po-merge.py (original)
> >>+++ subversion/trunk/tools/dev/po-merge.py Tue Jul 2 14:12:44 2013
> >>@@ -178,9 +178,10 @@ def main(argv):
> >> for i in msgstr:
> >> outfile.write('msgstr[%s] %s\n' % (n, msgstr[n]))
> >> n += 1
> >>- for m in msgstr:
> >>- if m == '""':
> >>- untranslated += 1
> >>+ if msgstr is not None:
> >>+ for m in msgstr:
> >>+ if m == '""':
> >>+ untranslated += 1
> >> for c in comments:
> >> if c.startswith('#,') and 'fuzzy' in c.split(', '):
> >> fuzzy += 1
> >>
> >>
>
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Dongsheng Song <do...@gmail.com>.
On 2013/7/3 4:05, Andreas Stieger wrote:
> Hi There,
>
> On 02/07/13 16:00, Dongsheng Song wrote:
>> Today, when I merge zh_CN.po from trunk to 1.8.x, I had encounter the
>> following error:
>>
>> $ python ../../../../trunk/tools/dev/po-merge.py <
>> ../../../../trunk/subversion/po/zh_CN.po zh_CN.po
>> Traceback (most recent call last):
>> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
>> main(sys.argv)
>> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
>> for m in msgstr:
>> TypeError: 'NoneType' object is not iterable
>>
>> Then I found in the line 39-40 of po-merge.py return None as msgstr:
>>
>> if line.strip() == '' or line[:2] == '#~':
>> return comments, None, None, None
>>
>> So we should not do iteration on msgstr without make sure msgstr is
>> not None.
>>
>> This happened because zh_CN.po have msgmerged po comments like this:
>>
>> #~ msgid "Uncommitted local addition, copy or move%s"
>> #~ msgstr "未提交的本地增加，复制或移动 %s"
>>
>> As your judgement, this is not "obvious fix", should I revert this
>> commit ?
> I cannot make sense of this change other than when malformed input files
> are concerned. Your example "^#~" requires msgstr == None == msgid as
> per the return of parse_translation(). That, then, means that comments
> evaluates to true (has entries) for the break in line 153 not to trigger.
>
> Can you give to input files (URl/revisions) that trigger this? So far
> this is my best guess:
>
> #SOMETHING
> #~ msgid "some msgid"
> #~ msgstr "some msgstr"
>
> I agree that msgstr == None should not be iterated, however I don't see
> how we get to this case.
>
> Andreas
I lost the intermediate file at yesterday, but I can reproduce it by another way.
$ cd subversion/branches/1.8.x/subversion/po
$ cat << EOF >> zh_CN.po
#, fuzzy
#~ msgid ""
#~ "use format compatible with Subversion versions\n"
#~ " earlier than 1.8"
#~ msgstr "使用与1.4之前版本兼容的格式"
EOF
$ python ../../../../trunk/tools/dev/po-merge.py < ../../../../trunk/subversion/po/zh_CN.po zh_CN.po
0 strings updated. 179 fuzzy strings. 138 of 2546 strings are still untranslated (5%).
$ tail zh_CN.po
msgid "'%s' is of unknown type\n"
msgstr "“%s” 的类型未知\n"
#. Local uncommitted modifications, no revision info was found.
#: ../svnversion/svnversion.c:273
#, c-format
msgid "Uncommitted local addition, copy or move%s"
msgstr "未提交的本地增加，复制或移动 %s"
#, fuzzy
!!! got a malformed po file !!!
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Daniel Shahaf <da...@elego.de>.
Dongsheng Song wrote on Wed, Jul 03, 2013 at 14:26:25 +0800:
> On 2013/7/3 6:55, Konstantin Kolinko wrote:
> > 2013/7/3 Andreas Stieger <an...@gmx.de>:
> >> Hi There,
> >>
> >> On 02/07/13 16:00, Dongsheng Song wrote:
> >>> Today, when I merge zh_CN.po from trunk to 1.8.x, I had encounter the
> >>> following error:
> >>>
> >>> $ python ../../../../trunk/tools/dev/po-merge.py <
> >>> ../../../../trunk/subversion/po/zh_CN.po zh_CN.po
> >>> Traceback (most recent call last):
> >>> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
> >>> main(sys.argv)
> >>> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
> >>> for m in msgstr:
> >>> TypeError: 'NoneType' object is not iterable
> >>>
> >>> Then I found in the line 39-40 of po-merge.py return None as msgstr:
> >>>
> >>> if line.strip() == '' or line[:2] == '#~':
> >>> return comments, None, None, None
> >>>
> >>> So we should not do iteration on msgstr without make sure msgstr is
> >>> not None.
> >>>
> >>> This happened because zh_CN.po have msgmerged po comments like this:
> >>>
> >>> #~ msgid "Uncommitted local addition, copy or move%s"
> >>> #~ msgstr "未提交的本地增加，复制或移动 %s"
> >>>
> >>> As your judgement, this is not "obvious fix", should I revert this
> >>> commit ?
> >> I cannot make sense of this change other than when malformed input files
> >> are concerned. Your example "^#~" requires msgstr == None == msgid as
> >> per the return of parse_translation(). That, then, means that comments
> >> evaluates to true (has entries) for the break in line 153 not to trigger.
> >>
> >> Can you give to input files (URl/revisions) that trigger this? So far
> >> this is my best guess:
> >>
> >> #SOMETHING
> >> #~ msgid "some msgid"
> >> #~ msgstr "some msgstr"
> >>
> >> I agree that msgstr == None should not be iterated, however I don't see
> >> how we get to this case.
> >>
> > Just noting:
> > The documentation string for "parse_translation(f)" function
> > explicitly documents what returned values can be None. The msgstr is
> > not one of them, it says "The msgstr is a list of strings.".
> >
> > But the actual implementation has one return statement that returns
> > None for that value.
> >
> > 39 arfrever 876651 > if line.strip() == '' or line[:2] == '#~':
> > 40 arfrever 874551 > return comments, None, None, None
> >
> > If you are going on with r1498947 then I think it would be better to
> > update the docstring.
> >
> > Alternatively, returning an empty array instead of the last 'None'
> > should be an other way to fix this issue.
> >
> > Best regards,
> > Konstantin Kolinko
> Yes, your noting looks more pretty, here is the patch:
>
> --- po-merge.py (revision 1499219)
> +++ po-merge.py (working copy)
> @@ -28,7 +28,7 @@
> """Read a single translation entry from the file F and return a
> tuple with the comments, msgid, msgid_plural and msgstr. The comments is
> returned as a list of lines which do not end in new-lines. The msgid is
> - string. The msgid_plural is string or None. The msgstr is a list of
> + string or None. The msgid_plural is string or None. The msgstr is a list of
> strings. The msgid, msgid_plural and msgstr strings can contain embedded
> newlines"""
> line = f.readline()
> @@ -37,7 +37,7 @@
> comments = []
> while True:
> if line.strip() == '' or line[:2] == '#~':
> - return comments, None, None, None
> + return comments, None, None, []
> elif line[0] == '#':
> comments.append(line[:-1])
> else:
> @@ -178,17 +178,16 @@
> for i in msgstr:
> outfile.write('msgstr[%s] %s\n' % (n, msgstr[n]))
> n += 1
> - if msgstr is not None:
> - for m in msgstr:
> - if m == '""':
> - untranslated += 1
> + for m in msgstr:
> + if m == '""':
> + untranslated += 1
+1 to commit (except the whitespace-only change at the end). Does this
fix the "invalid po file created" problem you ran into? Does that
problem even need to be fixed (I understand po-merge.py may be replaced
soon)?
Thanks Konstantin for the review.
Daniel
> for c in comments:
> if c.startswith('#,') and 'fuzzy' in c.split(', '):
> fuzzy += 1
>
> # We're done. Tell the user what we did.
> print(('%d strings updated. '
> - '%d fuzzy strings. '
> + '%d fuzzy strings. '
> '%d of %d strings are still untranslated (%.0f%%).' %
> (update_count, fuzzy, untranslated, string_count,
> 100.0 * untranslated / string_count)))
>
>
>
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Dongsheng Song <do...@gmail.com>.
于 2013/7/3 18:11, Konstantin Kolinko 写道:
> 2013/7/3 Dongsheng Song <do...@gmail.com>:
>> On 2013/7/3 6:55, Konstantin Kolinko wrote:
>>> 2013/7/3 Andreas Stieger <an...@gmx.de>:
>>>> Hi There,
>>>>
>>>> On 02/07/13 16:00, Dongsheng Song wrote:
>>>>> Today, when I merge zh_CN.po from trunk to 1.8.x, I had encounter the
>>>>> following error:
>>>>>
>>>>> $ python ../../../../trunk/tools/dev/po-merge.py <
>>>>> ../../../../trunk/subversion/po/zh_CN.po zh_CN.po
>>>>> Traceback (most recent call last):
>>>>> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
>>>>> main(sys.argv)
>>>>> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
>>>>> for m in msgstr:
>>>>> TypeError: 'NoneType' object is not iterable
>>>>>
>>>>> Then I found in the line 39-40 of po-merge.py return None as msgstr:
>>>>>
>>>>> if line.strip() == '' or line[:2] == '#~':
>>>>> return comments, None, None, None
>>>>>
>>>>> So we should not do iteration on msgstr without make sure msgstr is
>>>>> not None.
>>>>>
>>>>> This happened because zh_CN.po have msgmerged po comments like this:
>>>>>
>>>>> #~ msgid "Uncommitted local addition, copy or move%s"
>>>>> #~ msgstr "未提交的本地增加，复制或移动 %s"
>>>>>
>>>>> As your judgement, this is not "obvious fix", should I revert this
>>>>> commit ?
>>>> I cannot make sense of this change other than when malformed input files
>>>> are concerned. Your example "^#~" requires msgstr == None == msgid as
>>>> per the return of parse_translation(). That, then, means that comments
>>>> evaluates to true (has entries) for the break in line 153 not to trigger.
>>>>
>>>> Can you give to input files (URl/revisions) that trigger this? So far
>>>> this is my best guess:
>>>>
>>>> #SOMETHING
>>>> #~ msgid "some msgid"
>>>> #~ msgstr "some msgstr"
>>>>
>>>> I agree that msgstr == None should not be iterated, however I don't see
>>>> how we get to this case.
>>>>
>>> Just noting:
>>> The documentation string for "parse_translation(f)" function
>>> explicitly documents what returned values can be None. The msgstr is
>>> not one of them, it says "The msgstr is a list of strings.".
>>>
>>> But the actual implementation has one return statement that returns
>>> None for that value.
>>>
>>> 39 arfrever 876651 > if line.strip() == '' or line[:2] == '#~':
>>> 40 arfrever 874551 > return comments, None, None, None
>>>
>>> If you are going on with r1498947 then I think it would be better to
>>> update the docstring.
>>>
>>> Alternatively, returning an empty array instead of the last 'None'
>>> should be an other way to fix this issue.
>>>
>>> Best regards,
>>> Konstantin Kolinko
>> Yes, your noting looks more pretty, here is the patch:
>>
>> --- po-merge.py (revision 1499219)
>> +++ po-merge.py (working copy)
>> @@ -28,7 +28,7 @@
>> """Read a single translation entry from the file F and return a
>> tuple with the comments, msgid, msgid_plural and msgstr. The comments is
>> returned as a list of lines which do not end in new-lines. The msgid is
>> - string. The msgid_plural is string or None. The msgstr is a list of
>> + string or None. The msgid_plural is string or None. The msgstr is a list of
> You fix the method to never return None here. Thus there is no need to
> update the docstring.
No. The docstring which I fix is msgid, which maybe retrun None, but
docstring not said.
>> strings. The msgid, msgid_plural and msgstr strings can contain embedded
>> newlines"""
>> line = f.readline()
>> @@ -37,7 +37,7 @@
>> comments = []
>> while True:
>> if line.strip() == '' or line[:2] == '#~':
>> - return comments, None, None, None
>> + return comments, None, None, []
>> elif line[0] == '#':
>> comments.append(line[:-1])
>> else:
>> @@ -178,17 +178,16 @@
>> for i in msgstr:
>> outfile.write('msgstr[%s] %s\n' % (n, msgstr[n]))
>> n += 1
>> - if msgstr is not None:
>> - for m in msgstr:
>> - if m == '""':
>> - untranslated += 1
>> + for m in msgstr:
>> + if m == '""':
>> + untranslated += 1
>> for c in comments:
>> if c.startswith('#,') and 'fuzzy' in c.split(', '):
>> fuzzy += 1
>>
>> # We're done. Tell the user what we did.
>> print(('%d strings updated. '
>> - '%d fuzzy strings. '
>> + '%d fuzzy strings. '
> What is the change in the above line? I do not see any difference from
> this diff.
Me too. Then I view as hex, I found it's a strip of tailing space.
Regards,
Dongsheng
>> '%d of %d strings are still untranslated (%.0f%%).' %
>> (update_count, fuzzy, untranslated, string_count,
>> 100.0 * untranslated / string_count)))
>>
> Best regards,
> Konstantin Kolinko
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/7/3 Dongsheng Song <do...@gmail.com>:
> On 2013/7/3 6:55, Konstantin Kolinko wrote:
>> 2013/7/3 Andreas Stieger <an...@gmx.de>:
>>> Hi There,
>>>
>>> On 02/07/13 16:00, Dongsheng Song wrote:
>>>> Today, when I merge zh_CN.po from trunk to 1.8.x, I had encounter the
>>>> following error:
>>>>
>>>> $ python ../../../../trunk/tools/dev/po-merge.py <
>>>> ../../../../trunk/subversion/po/zh_CN.po zh_CN.po
>>>> Traceback (most recent call last):
>>>> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
>>>> main(sys.argv)
>>>> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
>>>> for m in msgstr:
>>>> TypeError: 'NoneType' object is not iterable
>>>>
>>>> Then I found in the line 39-40 of po-merge.py return None as msgstr:
>>>>
>>>> if line.strip() == '' or line[:2] == '#~':
>>>> return comments, None, None, None
>>>>
>>>> So we should not do iteration on msgstr without make sure msgstr is
>>>> not None.
>>>>
>>>> This happened because zh_CN.po have msgmerged po comments like this:
>>>>
>>>> #~ msgid "Uncommitted local addition, copy or move%s"
>>>> #~ msgstr "未提交的本地增加，复制或移动 %s"
>>>>
>>>> As your judgement, this is not "obvious fix", should I revert this
>>>> commit ?
>>> I cannot make sense of this change other than when malformed input files
>>> are concerned. Your example "^#~" requires msgstr == None == msgid as
>>> per the return of parse_translation(). That, then, means that comments
>>> evaluates to true (has entries) for the break in line 153 not to trigger.
>>>
>>> Can you give to input files (URl/revisions) that trigger this? So far
>>> this is my best guess:
>>>
>>> #SOMETHING
>>> #~ msgid "some msgid"
>>> #~ msgstr "some msgstr"
>>>
>>> I agree that msgstr == None should not be iterated, however I don't see
>>> how we get to this case.
>>>
>> Just noting:
>> The documentation string for "parse_translation(f)" function
>> explicitly documents what returned values can be None. The msgstr is
>> not one of them, it says "The msgstr is a list of strings.".
>>
>> But the actual implementation has one return statement that returns
>> None for that value.
>>
>> 39 arfrever 876651 > if line.strip() == '' or line[:2] == '#~':
>> 40 arfrever 874551 > return comments, None, None, None
>>
>> If you are going on with r1498947 then I think it would be better to
>> update the docstring.
>>
>> Alternatively, returning an empty array instead of the last 'None'
>> should be an other way to fix this issue.
>>
>> Best regards,
>> Konstantin Kolinko
> Yes, your noting looks more pretty, here is the patch:
>
> --- po-merge.py (revision 1499219)
> +++ po-merge.py (working copy)
> @@ -28,7 +28,7 @@
> """Read a single translation entry from the file F and return a
> tuple with the comments, msgid, msgid_plural and msgstr. The comments is
> returned as a list of lines which do not end in new-lines. The msgid is
> - string. The msgid_plural is string or None. The msgstr is a list of
> + string or None. The msgid_plural is string or None. The msgstr is a list of
You fix the method to never return None here. Thus there is no need to
update the docstring.
> strings. The msgid, msgid_plural and msgstr strings can contain embedded
> newlines"""
> line = f.readline()
> @@ -37,7 +37,7 @@
> comments = []
> while True:
> if line.strip() == '' or line[:2] == '#~':
> - return comments, None, None, None
> + return comments, None, None, []
> elif line[0] == '#':
> comments.append(line[:-1])
> else:
> @@ -178,17 +178,16 @@
> for i in msgstr:
> outfile.write('msgstr[%s] %s\n' % (n, msgstr[n]))
> n += 1
> - if msgstr is not None:
> - for m in msgstr:
> - if m == '""':
> - untranslated += 1
> + for m in msgstr:
> + if m == '""':
> + untranslated += 1
> for c in comments:
> if c.startswith('#,') and 'fuzzy' in c.split(', '):
> fuzzy += 1
>
> # We're done. Tell the user what we did.
> print(('%d strings updated. '
> - '%d fuzzy strings. '
> + '%d fuzzy strings. '
What is the change in the above line? I do not see any difference from
this diff.
> '%d of %d strings are still untranslated (%.0f%%).' %
> (update_count, fuzzy, untranslated, string_count,
> 100.0 * untranslated / string_count)))
>
Best regards,
Konstantin Kolinko
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Dongsheng Song <do...@gmail.com>.
On 2013/7/3 6:55, Konstantin Kolinko wrote:
> 2013/7/3 Andreas Stieger <an...@gmx.de>:
>> Hi There,
>>
>> On 02/07/13 16:00, Dongsheng Song wrote:
>>> Today, when I merge zh_CN.po from trunk to 1.8.x, I had encounter the
>>> following error:
>>>
>>> $ python ../../../../trunk/tools/dev/po-merge.py <
>>> ../../../../trunk/subversion/po/zh_CN.po zh_CN.po
>>> Traceback (most recent call last):
>>> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
>>> main(sys.argv)
>>> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
>>> for m in msgstr:
>>> TypeError: 'NoneType' object is not iterable
>>>
>>> Then I found in the line 39-40 of po-merge.py return None as msgstr:
>>>
>>> if line.strip() == '' or line[:2] == '#~':
>>> return comments, None, None, None
>>>
>>> So we should not do iteration on msgstr without make sure msgstr is
>>> not None.
>>>
>>> This happened because zh_CN.po have msgmerged po comments like this:
>>>
>>> #~ msgid "Uncommitted local addition, copy or move%s"
>>> #~ msgstr "未提交的本地增加，复制或移动 %s"
>>>
>>> As your judgement, this is not "obvious fix", should I revert this
>>> commit ?
>> I cannot make sense of this change other than when malformed input files
>> are concerned. Your example "^#~" requires msgstr == None == msgid as
>> per the return of parse_translation(). That, then, means that comments
>> evaluates to true (has entries) for the break in line 153 not to trigger.
>>
>> Can you give to input files (URl/revisions) that trigger this? So far
>> this is my best guess:
>>
>> #SOMETHING
>> #~ msgid "some msgid"
>> #~ msgstr "some msgstr"
>>
>> I agree that msgstr == None should not be iterated, however I don't see
>> how we get to this case.
>>
> Just noting:
> The documentation string for "parse_translation(f)" function
> explicitly documents what returned values can be None. The msgstr is
> not one of them, it says "The msgstr is a list of strings.".
>
> But the actual implementation has one return statement that returns
> None for that value.
>
> 39 arfrever 876651 > if line.strip() == '' or line[:2] == '#~':
> 40 arfrever 874551 > return comments, None, None, None
>
> If you are going on with r1498947 then I think it would be better to
> update the docstring.
>
> Alternatively, returning an empty array instead of the last 'None'
> should be an other way to fix this issue.
>
> Best regards,
> Konstantin Kolinko
Yes, your noting looks more pretty, here is the patch:
--- po-merge.py (revision 1499219)
+++ po-merge.py (working copy)
@@ -28,7 +28,7 @@
"""Read a single translation entry from the file F and return a
tuple with the comments, msgid, msgid_plural and msgstr. The comments is
returned as a list of lines which do not end in new-lines. The msgid is
- string. The msgid_plural is string or None. The msgstr is a list of
+ string or None. The msgid_plural is string or None. The msgstr is a list of
strings. The msgid, msgid_plural and msgstr strings can contain embedded
newlines"""
line = f.readline()
@@ -37,7 +37,7 @@
comments = []
while True:
if line.strip() == '' or line[:2] == '#~':
- return comments, None, None, None
+ return comments, None, None, []
elif line[0] == '#':
comments.append(line[:-1])
else:
@@ -178,17 +178,16 @@
for i in msgstr:
outfile.write('msgstr[%s] %s\n' % (n, msgstr[n]))
n += 1
- if msgstr is not None:
- for m in msgstr:
- if m == '""':
- untranslated += 1
+ for m in msgstr:
+ if m == '""':
+ untranslated += 1
for c in comments:
if c.startswith('#,') and 'fuzzy' in c.split(', '):
fuzzy += 1
# We're done. Tell the user what we did.
print(('%d strings updated. '
- '%d fuzzy strings. '
+ '%d fuzzy strings. '
'%d of %d strings are still untranslated (%.0f%%).' %
(update_count, fuzzy, untranslated, string_count,
100.0 * untranslated / string_count)))
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/7/3 Andreas Stieger <an...@gmx.de>:
> Hi There,
>
> On 02/07/13 16:00, Dongsheng Song wrote:
>> Today, when I merge zh_CN.po from trunk to 1.8.x, I had encounter the
>> following error:
>>
>> $ python ../../../../trunk/tools/dev/po-merge.py <
>> ../../../../trunk/subversion/po/zh_CN.po zh_CN.po
>> Traceback (most recent call last):
>> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
>> main(sys.argv)
>> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
>> for m in msgstr:
>> TypeError: 'NoneType' object is not iterable
>>
>> Then I found in the line 39-40 of po-merge.py return None as msgstr:
>>
>> if line.strip() == '' or line[:2] == '#~':
>> return comments, None, None, None
>>
>> So we should not do iteration on msgstr without make sure msgstr is
>> not None.
>>
>> This happened because zh_CN.po have msgmerged po comments like this:
>>
>> #~ msgid "Uncommitted local addition, copy or move%s"
>> #~ msgstr "未提交的本地增加，复制或移动 %s"
>>
>> As your judgement, this is not "obvious fix", should I revert this
>> commit ?
>
> I cannot make sense of this change other than when malformed input files
> are concerned. Your example "^#~" requires msgstr == None == msgid as
> per the return of parse_translation(). That, then, means that comments
> evaluates to true (has entries) for the break in line 153 not to trigger.
>
> Can you give to input files (URl/revisions) that trigger this? So far
> this is my best guess:
>
> #SOMETHING
> #~ msgid "some msgid"
> #~ msgstr "some msgstr"
>
> I agree that msgstr == None should not be iterated, however I don't see
> how we get to this case.
>
Just noting:
The documentation string for "parse_translation(f)" function
explicitly documents what returned values can be None. The msgstr is
not one of them, it says "The msgstr is a list of strings.".
But the actual implementation has one return statement that returns
None for that value.
39 arfrever 876651 > if line.strip() == '' or line[:2] == '#~':
40 arfrever 874551 > return comments, None, None, None
If you are going on with r1498947 then I think it would be better to
update the docstring.
Alternatively, returning an empty array instead of the last 'None'
should be an other way to fix this issue.
Best regards,
Konstantin Kolinko
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Andreas Stieger <an...@gmx.de>.
Hi There,
On 02/07/13 16:00, Dongsheng Song wrote:
> Today, when I merge zh_CN.po from trunk to 1.8.x, I had encounter the
> following error:
>
> $ python ../../../../trunk/tools/dev/po-merge.py <
> ../../../../trunk/subversion/po/zh_CN.po zh_CN.po
> Traceback (most recent call last):
> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
> main(sys.argv)
> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
> for m in msgstr:
> TypeError: 'NoneType' object is not iterable
>
> Then I found in the line 39-40 of po-merge.py return None as msgstr:
>
> if line.strip() == '' or line[:2] == '#~':
> return comments, None, None, None
>
> So we should not do iteration on msgstr without make sure msgstr is
> not None.
>
> This happened because zh_CN.po have msgmerged po comments like this:
>
> #~ msgid "Uncommitted local addition, copy or move%s"
> #~ msgstr "未提交的本地增加，复制或移动 %s"
>
> As your judgement, this is not "obvious fix", should I revert this
> commit ?
I cannot make sense of this change other than when malformed input files
are concerned. Your example "^#~" requires msgstr == None == msgid as
per the return of parse_translation(). That, then, means that comments
evaluates to true (has entries) for the break in line 153 not to trigger.
Can you give to input files (URl/revisions) that trigger this? So far
this is my best guess:
#SOMETHING
#~ msgid "some msgid"
#~ msgstr "some msgstr"
I agree that msgstr == None should not be iterated, however I don't see
how we get to this case.
Andreas
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Dongsheng Song <do...@gmail.com>.
于 2013/7/2 22:30, Daniel Shahaf 写道:
> dongsheng@apache.org wrote on Tue, Jul 02, 2013 at 14:12:44 -0000:
>> Author: dongsheng
>> Date: Tue Jul 2 14:12:44 2013
>> New Revision: 1498947
>>
>> URL: http://svn.apache.org/r1498947
>> Log:
>> Make sure msgstr is not None before do iteration.
>>
>> Obvious fix.
>>
> No, it's not an obvious fix. It's a code change, and one which is not
> obvious, so it requires review.
>
>> Since parse_translation maybe return (comments, None, None, None),
> When does that happen? Why is that not a bug? Why no translator run
> into it until today?
Today, when I merge zh_CN.po from trunk to 1.8.x, I had encounter the
following error:
$ python ../../../../trunk/tools/dev/po-merge.py <
../../../../trunk/subversion/po/zh_CN.po zh_CN.po
Traceback (most recent call last):
File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
main(sys.argv)
File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
for m in msgstr:
TypeError: 'NoneType' object is not iterable
Then I found in the line 39-40 of po-merge.py return None as msgstr:
if line.strip() == '' or line[:2] == '#~':
return comments, None, None, None
So we should not do iteration on msgstr without make sure msgstr is not
None.
This happened because zh_CN.po have msgmerged po comments like this:
#~ msgid "Uncommitted local addition, copy or move%s"
#~ msgstr "未提交的本地增加，复制或移动 %s"
As your judgement, this is not "obvious fix", should I revert this commit ?
> Thanks,
>
> Daniel
>
>> * tools/dev/po-merge.py
>> we should not do iteration without check msgstr is not None. Otherwise,
>> we may encounter the following TypeError:
>>
>> Traceback (most recent call last):
>> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
>> main(sys.argv)
>> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
>> for m in msgstr:
>> TypeError: 'NoneType' object is not iterable
>>
>> Modified:
>> subversion/trunk/tools/dev/po-merge.py
>>
>> Modified: subversion/trunk/tools/dev/po-merge.py
>> URL: http://svn.apache.org/viewvc/subversion/trunk/tools/dev/po-merge.py?rev=1498947&r1=1498946&r2=1498947&view=diff
>> ==============================================================================
>> --- subversion/trunk/tools/dev/po-merge.py (original)
>> +++ subversion/trunk/tools/dev/po-merge.py Tue Jul 2 14:12:44 2013
>> @@ -178,9 +178,10 @@ def main(argv):
>> for i in msgstr:
>> outfile.write('msgstr[%s] %s\n' % (n, msgstr[n]))
>> n += 1
>> - for m in msgstr:
>> - if m == '""':
>> - untranslated += 1
>> + if msgstr is not None:
>> + for m in msgstr:
>> + if m == '""':
>> + untranslated += 1
>> for c in comments:
>> if c.startswith('#,') and 'fuzzy' in c.split(', '):
>> fuzzy += 1
>>
>>
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Daniel Shahaf <da...@elego.de>.
dongsheng@apache.org wrote on Tue, Jul 02, 2013 at 14:12:44 -0000:
> Author: dongsheng
> Date: Tue Jul 2 14:12:44 2013
> New Revision: 1498947
>
> URL: http://svn.apache.org/r1498947
> Log:
> Make sure msgstr is not None before do iteration.
>
> Obvious fix.
>
No, it's not an obvious fix. It's a code change, and one which is not
obvious, so it requires review.
> Since parse_translation maybe return (comments, None, None, None),
When does that happen? Why is that not a bug? Why no translator run
into it until today?
Thanks,
Daniel
> * tools/dev/po-merge.py
> we should not do iteration without check msgstr is not None. Otherwise,
> we may encounter the following TypeError:
>
> Traceback (most recent call last):
> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
> main(sys.argv)
> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
> for m in msgstr:
> TypeError: 'NoneType' object is not iterable
>
> Modified:
> subversion/trunk/tools/dev/po-merge.py
>
> Modified: subversion/trunk/tools/dev/po-merge.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/tools/dev/po-merge.py?rev=1498947&r1=1498946&r2=1498947&view=diff
> ==============================================================================
> --- subversion/trunk/tools/dev/po-merge.py (original)
> +++ subversion/trunk/tools/dev/po-merge.py Tue Jul 2 14:12:44 2013
> @@ -178,9 +178,10 @@ def main(argv):
> for i in msgstr:
> outfile.write('msgstr[%s] %s\n' % (n, msgstr[n]))
> n += 1
> - for m in msgstr:
> - if m == '""':
> - untranslated += 1
> + if msgstr is not None:
> + for m in msgstr:
> + if m == '""':
> + untranslated += 1
> for c in comments:
> if c.startswith('#,') and 'fuzzy' in c.split(', '):
> fuzzy += 1
>
>
Re: svn commit: r1498947 - /subversion/trunk/tools/dev/po-merge.py
Posted by Daniel Shahaf <da...@elego.de>.
dongsheng@apache.org wrote on Tue, Jul 02, 2013 at 14:12:44 -0000:
> Author: dongsheng
> Date: Tue Jul 2 14:12:44 2013
> New Revision: 1498947
>
> URL: http://svn.apache.org/r1498947
> Log:
> Make sure msgstr is not None before do iteration.
>
> Obvious fix.
>
No, it's not an obvious fix. It's a code change, and one which is not
obvious, so it requires review.
> Since parse_translation maybe return (comments, None, None, None),
When does that happen? Why is that not a bug? Why no translator run
into it until today?
Thanks,
Daniel
> * tools/dev/po-merge.py
> we should not do iteration without check msgstr is not None. Otherwise,
> we may encounter the following TypeError:
>
> Traceback (most recent call last):
> File "../../../../trunk/tools/dev/po-merge.py", line 196, in <module>
> main(sys.argv)
> File "../../../../trunk/tools/dev/po-merge.py", line 181, in main
> for m in msgstr:
> TypeError: 'NoneType' object is not iterable
>
> Modified:
> subversion/trunk/tools/dev/po-merge.py
>
> Modified: subversion/trunk/tools/dev/po-merge.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/tools/dev/po-merge.py?rev=1498947&r1=1498946&r2=1498947&view=diff
> ==============================================================================
> --- subversion/trunk/tools/dev/po-merge.py (original)
> +++ subversion/trunk/tools/dev/po-merge.py Tue Jul 2 14:12:44 2013
> @@ -178,9 +178,10 @@ def main(argv):
> for i in msgstr:
> outfile.write('msgstr[%s] %s\n' % (n, msgstr[n]))
> n += 1
> - for m in msgstr:
> - if m == '""':
> - untranslated += 1
> + if msgstr is not None:
> + for m in msgstr:
> + if m == '""':
> + untranslated += 1
> for c in comments:
> if c.startswith('#,') and 'fuzzy' in c.split(', '):
> fuzzy += 1
>
>