launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve

One more edge case that you need to fix, but feel free to land once you've done that.  Thanks!

Diff comments:

> === modified file 'lib/lp/code/mail/codereviewcomment.py'
> --- lib/lp/code/mail/codereviewcomment.py	2014-05-21 08:52:53 +0000
> +++ lib/lp/code/mail/codereviewcomment.py	2015-06-26 02:37:24 +0000
> @@ -10,7 +10,7 @@
>      'CodeReviewCommentMailer',
>      ]
>  
> -
> +from bzrlib import patches
>  from zope.component import getUtility
>  from zope.security.proxy import removeSecurityProxy
>  
> @@ -169,18 +169,85 @@
>                  content, content_type=content_type, filename=filename)
>  
>  
> +def format_comment(comment):
> +    """Returns a list of correctly formatted comment(s)."""
> +    comment_lines = []
> +    if comment is not None:
> +        comment_lines.append('')
> +        comment_lines.extend(comment.splitlines())
> +        comment_lines.append('')
> +    return comment_lines
> +
> +
>  def build_inline_comments_section(comments, diff_text):
> -    """Return a formatted text section with contextualized comments."""
> +    """Return a formatted text section with contextualized comments.
> +
> +    Hunks without comments are skipped to limit verbosity.
> +    Comments can be rendered after patch headers, hunk context lines,
> +    and hunk lines.
> +    """
> +    diff_lines = diff_text.splitlines(True)
> +
> +    diff_patches = patches.parse_patches(
> +        diff_lines, allow_dirty=True, keep_dirty=True)
>      result_lines = []
> -    diff_lines = diff_text.splitlines()
> -    for num, line in enumerate(diff_lines, 1):
> -        result_lines.append(u'> {0}'.format(line.decode('utf-8', 'replace')))
> -        comment = comments.get(str(num))
> -        if comment is not None:
> -            result_lines.append('')
> -            result_lines.extend(comment.splitlines())
> -            result_lines.append('')
> -
> -    result_text = u'\n'.join(result_lines)
> -
> +    line_count = 0  # track lines in original diff
> +
> +    for patch in diff_patches:
> +        patch_lines = []
> +        dirty_head = []
> +        patch_comment = False
> +
> +        if isinstance(patch, dict) and 'dirty_head' in patch:
> +            for line in patch['dirty_head']:
> +                dirty_head.append(u'> %s' % line.rstrip('\n'))
> +                line_count += 1  # inc for dirty headers
> +            patch = patch['patch']
> +
> +        for ph in patch.get_header().splitlines():
> +            line_count += 1  # inc patch headers
> +            comment = comments.get(str(line_count))
> +
> +            patch_lines.append('> {0}'.format(ph))
> +            if comment:
> +                patch_lines.extend(format_comment(comment))
> +                patch_comment = True
> +
> +        keep_hunks = []  # preserve hunks with comments
> +        for hunk in patch.hunks:
> +            hunk_lines = []
> +            hunk_comment = False
> +
> +            # add context line (hunk header)
> +            line_count += 1  # inc hunk context line
> +            hunk_lines.append(u'> %s' % hunk.get_header().rstrip('\n'))
> +
> +            # comment for context line (hunk header)
> +            comment = comments.get(str(line_count))
> +            if comment:
> +                hunk_lines.extend(format_comment(comment))
> +                hunk_comment = True
> +
> +            for line in hunk.lines:
> +                line_count += 1  # inc hunk lines
> +
> +                #  line is a ContextLine/ReplaceLine
> +                hunk_lines.append(u'> %s' % str(line).rstrip('\n').decode(
> +                    'utf-8', 'replace'))
> +                comment = comments.get(str(line_count))
> +                if comment:
> +                    hunk_lines.extend(format_comment(comment))
> +                    hunk_comment = True
> +
> +            # preserve hunks for context if comment in patch header
> +            if patch_comment or hunk_comment:
> +                keep_hunks.extend(hunk_lines)
> +
> +        # Add entire patch and hunks to result if comment found
> +        if keep_hunks:

What about the case where a patch has no hunks, such as a merge proposal that changes the executable bit on a file but not its content, and there's a comment on the patch header?  In that case, patch_lines will be non-empty but keep_hunks will be empty.  I think "if patch_lines or keep_hunks:" would be better here.  (Again, this will need a test.)

> +            result_lines.extend(dirty_head)
> +            result_lines.extend(patch_lines)
> +            result_lines.extend(keep_hunks)
> +
> +    result_text = '\n'.join(result_lines)
>      return '\n\nDiff comments:\n\n%s\n\n' % result_text
> 
> === modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
> --- lib/lp/code/mail/tests/test_codereviewcomment.py	2014-08-19 04:56:39 +0000
> +++ lib/lp/code/mail/tests/test_codereviewcomment.py	2015-06-26 02:37:24 +0000
> @@ -239,7 +239,7 @@
>          See `build_inline_comments_section` tests for formatting details.
>          """
>          comment = self.makeCommentWithInlineComments(
> -            inline_comments={'2': u'Is this from Planet Earth\xa9 ?'})
> +            inline_comments={'3': 'Is this from Pl\u0060net Earth ?'})
>          mailer = CodeReviewCommentMailer.forCreation(comment)
>          commenter = comment.branch_merge_proposal.registrant
>          ctrl = mailer.generateEmail(
> @@ -249,14 +249,14 @@
>              '',
>              'Diff comments:',
>              '',
> -            "> === zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'",
> -            '> --- yvo/yc/pbqr/vagresnprf/qvss.cl      '
> -            '2009-10-01 13:25:12 +0000',
> -            '',
> -            u'Is this from Planet Earth\xa9 ?',
> -            '',
> -            '> +++ yvo/yc/pbqr/vagresnprf/qvss.cl      '
> -            '2010-02-02 15:48:56 +0000'
> +            ("> === zbqvsvrq svyr 'yvo/yc/pbqr/vagresnprf/qvss.cl'"),
> +            ('> --- yvo/yc/pbqr/vagresnprf/qvss.cl      '
> +             '2009-10-01 13:25:12 +0000'),
> +            ('> +++ yvo/yc/pbqr/vagresnprf/qvss.cl      '
> +             '2010-02-02 15:48:56 +0000'),
> +            '',
> +            'Is this from Pl\u0060net Earth ?',
> +            '',
>          ]
>          self.assertEqual(expected_lines, ctrl.body.splitlines()[1:10])
>  
> @@ -378,6 +378,8 @@
>      """Tests for `build_inline_comments_section`."""
>  
>      diff_text = (
> +        "=== added directory 'foo/bar'\n"
> +        "=== modified file 'foo/bar/baz.py'\n"
>          "--- bar\t2009-08-26 15:53:34.000000000 -0400\n"
>          "+++ bar\t1969-12-31 19:00:00.000000000 -0500\n"
>          "@@ -1,3 +0,0 @@\n"
> @@ -389,6 +391,9 @@
>          "@@ -0,0 +1,2 @@\n"
>          "+a\n"
>          "+b\n"
> +        "@@ -1,2 +0,0 @@\n"
> +        "-x\n"
> +        "-y\n"
>          "--- foo\t2009-08-26 15:53:23.000000000 -0400\n"
>          "+++ foo\t2009-08-26 15:56:43.000000000 -0400\n"
>          "@@ -1,3 +1,4 @@\n"
> @@ -408,48 +413,104 @@
>          # The inline comments section starts with a 4-lines header
>          # (empty lines and title) and ends with an empty line.
>          section = self.getSection({}).splitlines()
> -        header = section[:5]
> +        header = section[:4]
>          self.assertEqual(
>              ['',
>               '',
>               'Diff comments:',
> -             '',
> -             '> --- bar\t2009-08-26 15:53:34.000000000 -0400'],
> -            header)
> -        footer = section[-2:]
> +             ''], header)
> +        footer = section[-1:]
>          self.assertEqual(
> -            ['> +e',
> -             ''],
> +            [''],
>              footer)
>  
>      def test_single_line_comment(self):
>          # The inline comments are correctly contextualized in the diff.
>          # and prefixed with '>>> '
> -        comments = {'2': u'\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae'}
> -        self.assertEqual(
> -            ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
> -             '',
> -             u'\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae',
> -             '',
> -             '> @@ -1,3 +0,0 @@',
> -             u'> -\xe5'],
> -            self.getSection(comments).splitlines()[5:11])
> +        comments = {'4': '\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae'}
> +        self.assertEqual(
> +            map(unicode, ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
> +             '',
> +             '\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae',
> +             '']),
> +            self.getSection(comments).splitlines()[7:11])
> +
> +    def test_commentless_hunks_ignored(self):
> +        # Hunks without inline comments are not returned in the diff text.
> +        comments = {'16': 'A comment', '21': 'Another comment'}
> +        self.assertEqual(
> +            map(unicode, ['> --- baz\t1969-12-31 19:00:00.000000000 -0500',
> +             '> +++ baz\t2009-08-26 15:53:57.000000000 -0400',
> +             '> @@ -1,2 +0,0 @@',
> +             '> -x',
> +             '> -y',
> +             '',
> +             'A comment',
> +             '',
> +             '> --- foo\t2009-08-26 15:53:23.000000000 -0400',
> +             '> +++ foo\t2009-08-26 15:56:43.000000000 -0400',
> +             '> @@ -1,3 +1,4 @@',
> +             '>  a',
> +             '> -b',
> +             '',
> +             'Another comment',
> +             '',
> +             '>  c',
> +             '> +d',
> +             '> +e']),
> +            self.getSection(comments).splitlines()[4:23])
> +
> +    def test_patch_header_comment(self):
> +        # Inline comments in patch headers are rendered correctly and
> +        # include the patch's hunk(s).
> +        comments = {'17': 'A comment in the patch header', '18': 'aardvark'}
> +        self.assertEqual(
> +            map(unicode, [
> +                '> --- foo\t2009-08-26 15:53:23.000000000 -0400',
> +                '',
> +                'A comment in the patch header',
> +                '',
> +                '> +++ foo\t2009-08-26 15:56:43.000000000 -0400',
> +                '',
> +                'aardvark',
> +                '',
> +                '> @@ -1,3 +1,4 @@',
> +                '>  a',
> +                '> -b',
> +                '>  c',
> +                '> +d',
> +                '> +e']),
> +            self.getSection(comments).splitlines()[4:18])
> +
> +    def test_non_last_hunk_comment(self):
> +        comments = {'12': 'A comment in the non-last hunk'}
> +        self.assertEqual(
> +            map(unicode, [
> +                '> --- baz\t1969-12-31 19:00:00.000000000 -0500',
> +                '> +++ baz\t2009-08-26 15:53:57.000000000 -0400',
> +                '> @@ -0,0 +1,2 @@',
> +                '> +a',
> +                '',
> +                'A comment in the non-last hunk',
> +                '',
> +                '> +b']),
> +            self.getSection(comments).splitlines()[4:12])
>  
>      def test_multi_line_comment(self):
>          # Inline comments with multiple lines are rendered appropriately.
> -        comments = {'2': 'Foo\nBar'}
> +        comments = {'4': 'Foo\nBar'}
>          self.assertEqual(
> -            ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
> +            map(unicode, ['> --- bar\t2009-08-26 15:53:34.000000000 -0400',
> +             '> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
>               '',
>               'Foo',
>               'Bar',
> -             '',
> -             '> @@ -1,3 +0,0 @@'],
> -            self.getSection(comments).splitlines()[5:11])
> +             '']),
> +            self.getSection(comments).splitlines()[6:12])
>  
>      def test_multiple_comments(self):
>          # Multiple inline comments are redered appropriately.
> -        comments = {'2': 'Foo', '3': 'Bar'}
> +        comments = {'4': 'Foo', '5': 'Bar'}
>          self.assertEqual(
>              ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
>               '',
> @@ -459,4 +520,4 @@
>               '',
>               'Bar',
>               ''],
> -            self.getSection(comments).splitlines()[5:13])
> +            self.getSection(comments).splitlines()[7:15])
> 
> === modified file 'versions.cfg'
> --- versions.cfg	2015-06-24 07:45:11 +0000
> +++ versions.cfg	2015-06-26 02:37:24 +0000
> @@ -16,7 +16,7 @@
>  auditorfixture = 0.0.5
>  BeautifulSoup = 3.2.1
>  bson = 0.3.3
> -bzr = 2.6.0
> +bzr = 2.6.0.lp.1
>  celery = 2.5.1
>  Chameleon = 2.11
>  cssselect = 0.9.1
> 


-- 
https://code.launchpad.net/~blr/launchpad/bug-1334577-verbose-diff/+merge/243751
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.