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 <andreas.stie...@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)))
>
>
>
