launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18859
Re: [Merge] lp:~blr/launchpad/bug-1334577-verbose-diff into lp:launchpad
Review: Approve
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:
I meant patch_comment, yes.
> + 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.
References
