launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/code/mail/codereviewcomment.py'
> === modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
> --- lib/lp/code/mail/codereviewcomment.py	2015-06-30 08:51:21 +0000
> +++ lib/lp/code/mail/codereviewcomment.py	2015-07-07 04:25:43 +0000
> @@ -209,6 +209,17 @@
>                      dirty_comment = True
>              patch = patch['patch']
>  

This needs a comment explaining why it isn't isinstance.

And shouldn't it be dedented? It needs to fire even without a dirty_head.

> +            if type(patch) is patches.BinaryPatch:
> +                result_lines.extend(dirty_head)
> +                result_lines.append(
> +                    '> Binary files {old} and {new} differ'.format(

This looks like result_lines.append(str(patch))

> +                        old=patch.oldname, new=patch.newname))
> +                line_count += 1
> +                comment = comments.get(str(line_count))
> +                if comment:
> +                    result_lines.extend(format_comment(comment))
> +                continue
> +
>          for ph in patch.get_header().splitlines():
>              line_count += 1  # inc patch headers
>              comment = comments.get(str(line_count))


-- 
https://code.launchpad.net/~blr/launchpad/bug-1471426-commentmail-binarypatch-oops/+merge/263995
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.