launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Review: Approve

Thanks for working on this.

I have only on minor comment about replacing L.insert(0,...) for a append(''); extend(...); append('')

Once that's done we can land it.

Inline comments:

> --- lib/lp/app/javascript/inlineedit/assets/skins/sam/editor-skin.css	2014-04-09 19:58:33 +0000
> +++ lib/lp/app/javascript/inlineedit/assets/skins/sam/editor-skin.css	2014-05-12 23:05:37 +0000
> @@ -224,7 +224,7 @@
>    font-family: Ubuntu,"Bitstream Vera Sans","DejaVu Sans",Tahoma,sans-serif;
>    font-size: 12px;
>    padding: 0 12px 0;
> -  word-break: break-word;
> +  word-break: break-all;
>  }
>  
>  .inline-comment>div { line-height: 25px; }
> 
> === modified file 'lib/lp/code/mail/codereviewcomment.py'
> --- lib/lp/code/mail/codereviewcomment.py	2014-04-01 02:14:58 +0000
> +++ lib/lp/code/mail/codereviewcomment.py	2014-05-12 23:05:37 +0000
> @@ -177,7 +177,10 @@
>          result_lines.append('> {0}'.format(diff_lines[i]))
>          comment = comments.get(str(i))
>          if comment is not None:
> -            result_lines.extend(comment.splitlines())
> +            com = comment.splitlines()
> +            com.insert(0, '')
> +            com.append('')
> +            result_lines.extend(com)
>  
I think it would be clearer as:
{{{
+ results_lines.append('')
   results_lines.extend(comment.splitlines())
+ result_lines.append('')
}}}
The insert(0, ...) is confusing to me (indexing feels like C)
>      result_text = '\n'.join(result_lines)
>  
> 
> === modified file 'lib/lp/code/mail/tests/test_codereviewcomment.py'
> --- lib/lp/code/mail/tests/test_codereviewcomment.py	2014-03-26 19:29:20 +0000
> +++ lib/lp/code/mail/tests/test_codereviewcomment.py	2014-05-12 23:05:37 +0000
> @@ -247,7 +247,7 @@
>          comment = self.makeCommentWithInlineComments(
>              inline_comments={'1': u'Is this from Planet Earth\xa9 ?'})
>          mailer = CodeReviewCommentMailer.forCreation(comment)
> -        commenter = comment.branch_merge_proposal.registrant 
> +        commenter = comment.branch_merge_proposal.registrant
>          ctrl = mailer.generateEmail(
>              commenter.preferredemail.email, commenter)
>  
> @@ -257,11 +257,13 @@
>              '',
>              '> --- 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'
>          ]
> -        self.assertEqual(expected_lines, ctrl.body.splitlines()[1:7])
> +        self.assertEqual(expected_lines, ctrl.body.splitlines()[1:9])
>  
>      def test_generateEmailWithInlineComments_feature_disabled(self):
>          """Inline comments are not considered if the flag is not enabled."""
> @@ -270,7 +272,7 @@
>          content = 'CoNtEnT'
>          comment = self.makeCommentWithInlineComments(content=content)
>          mailer = CodeReviewCommentMailer.forCreation(comment)
> -        commenter = comment.branch_merge_proposal.registrant 
> +        commenter = comment.branch_merge_proposal.registrant
>          ctrl = mailer.generateEmail(
>              commenter.preferredemail.email, commenter)
>          # Only the comment content (footer is ignored) is included in
> @@ -425,7 +427,7 @@
>      def test_section_header_and_footer(self):
>          # The inline comments section starts with a 4-lines header
>          # (empty lines and title) and ends with an empty line.
> -        section = self.getSection({}).splitlines() 
> +        section = self.getSection({}).splitlines()
>          header = section[:5]
>          self.assertEqual(
>              ['',
> @@ -452,7 +454,7 @@
>              self.getSection(comments).splitlines()[4:8])
>  
>      def test_multi_line_comment(self):
> -        # Inline comments with multiple lines are rendered appropriately. 
> +        # Inline comments with multiple lines are rendered appropriately.
>          comments = {'1': 'Foo\nBar'}
>          self.assertEqual(
>              ['> +++ bar\t1969-12-31 19:00:00.000000000 -0500',
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/fix-word-break/+merge/219252
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.