launchpad-reviewers team mailing list archive

[Chris Johnston <chris.johnston@xxxxxxxxxxxxx>]

Chris Johnston has proposed merging lp:~cjohnston/launchpad/short-ic-emails into lp:launchpad.

Commit message:
First pass at reducing the amount of code that is emailed out when someone saves diff comments

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjohnston/launchpad/short-ic-emails/+merge/225095

This branch makes a first pass at reducing the amount of code that is emailed out when someone saves diff comments. The way this is done is by discarding hunks that do not have diff comments.
-- 
https://code.launchpad.net/~cjohnston/launchpad/short-ic-emails/+merge/225095
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjohnston/launchpad/short-ic-emails into lp:launchpad.

=== 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	2014-06-30 23:09:28 +0000
@@ -11,6 +11,7 @@
     ]
 
 
+from bzrlib.patches import parse_patches
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -171,16 +172,37 @@
 
 def build_inline_comments_section(comments, diff_text):
     """Return a formatted text section with contextualized comments."""
-    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('')
+    hunk_code = []
+    patches = parse_patches(diff_text.splitlines(True), allow_dirty=True)
+    line_count = 0
+    for patch in patches:
+        # file
+        patch_code = []
+        patch_has_ic = False
+        headers = patch.get_header().strip()
+        line_count += len(headers.split('\n'))
+        patch_code.append(headers)
+        for p in patch.hunks:
+            # hunk in file
+            this_hunk_code = []
+            has_ic = False
+            diff_code = str(p).split('\n')
+            for num, line in enumerate(diff_code, 1):
+                line_count += 1
+                this_hunk_code.append(u'> {0}'.format(
+                    line.decode('utf-8', 'replace')))
+                comment = comments.get(str(line_count))
+                if comment is not None:
+                    this_hunk_code.append('')
+                    this_hunk_code.extend(comment.splitlines())
+                    this_hunk_code.append('')
+                    has_ic = True
+            if has_ic:
+                patch_code += this_hunk_code
+                patch_has_ic = True
+        if patch_has_ic:
+            hunk_code += patch_code
 
-    result_text = u'\n'.join(result_lines)
+    result_text = u'\n'.join(hunk_code)
 
     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-05-21 09:33:04 +0000
+++ lib/lp/code/mail/tests/test_codereviewcomment.py	2014-06-30 23:09:28 +0000
@@ -30,6 +30,81 @@
     )
 from lp.testing.layers import LaunchpadFunctionalLayer
 
+expected_lines = [
+    u"",
+    u"",
+    u"Diff comments:",
+    u"",
+    u"--- charms/precise/python-django/config.yaml    "
+    u"2014-05-30 14:46:01 +0000",
+    u"+++ charms/precise/python-django/config.yaml    "
+    u"2014-06-06 18:15:02 +0000",
+    u"> @@ -67,6 +67,10 @@",
+    u">          type: string",
+    u">          default: '/srv/'",
+    u">          description: The root directory to checkout to.",
+    u"> +    instance_type:",
+    u"> +        type: string",
+    u"> +        default: 'development'",
+    u"> +        description: 'Instance type (development, staging, or "
+    u"production)'",
+    u"",
+    u"Are we in development\xa9 ?",
+    u"",
+    u">      application_path:",
+    u">          type: string",
+    u">          default: ''",
+    u"> ",
+    u"--- charms/precise/python-django/hooks/hooks.py "
+    u"2014-06-05 15:32:53 +0000",
+    u"+++ charms/precise/python-django/hooks/hooks.py "
+    u"2014-06-06 23:38:36 +0000",
+    u"> @@ -919,19 +926,21 @@",
+    u">  django_south = config_data['django_south']",
+    u">  if django_south:",
+    u">      CHARM_PACKAGES.append('python-django-south')",
+    u"> -",
+    u"> +instance_type = config_data['instance_type']",
+    u"",
+    u"Is this development\xa9 ?",
+    u"",
+    u">  unit_name = os.environ['JUJU_UNIT_NAME'].split('/')[0]",
+    u">  sanitized_unit_name = sanitize(unit_name)",
+    u"> -vcs_clone_dir = os.path.join(install_root, sanitized_unit_name)",
+    u"> +base_dir = os.path.join(install_root, sanitized_unit_name)",
+    u"> +django_settings_modules = '.'.join([sanitized_unit_name, "
+    u"django_settings])",
+    u"> +django_settings_modules = django_settings  # andy hack",
+    u"> +django_run_dir = os.path.join(base_dir, instance_type + '-run')",
+    u"> +django_logs_dir = os.path.join(base_dir, instance_type + '-logs')",
+    u"> +if config_data['wsgi_log_file'] or config_data["
+    u"'wsgi_access_logfile']:",
+    u"> +    install_dir(django_logs_dir, owner=wsgi_user, group=wsgi_group, "
+    u"mode=0755)",
+    u"> +vcs_clone_dir = os.path.join(base_dir, instance_type)",
+    u">  if application_path:",
+    u">      working_dir = os.path.join(vcs_clone_dir, application_path)",
+    u">  else:",
+    u">      working_dir = vcs_clone_dir",
+    u"> -",
+    u"> -django_settings_modules = '.'.join([sanitized_unit_name, "
+    u"django_settings])",
+    u"> -django_settings_modules = django_settings  # andy hack",
+    u"> -django_run_dir = os.path.join(working_dir, 'run/')",
+    u"> -django_logs_dir = os.path.join(working_dir, 'logs/')",
+    u">  settings_py_path = os.path.join(working_dir, 'settings.py')",
+    u">  urls_py_path = os.path.join(working_dir, 'urls.py')",
+    u">  settings_dir_path = os.path.join(",
+    u"> ",
+    u"",
+    u"",
+    u"-- ",
+    u"http://code.launchpad.dev/~person-name-100011/product-name-100002/";
+    u"branch-100009/+merge/1",
+    u"You proposed lp://dev/~person-name-100011/product-name-100002/"
+    u"branch-100009 for merging."
+]
+
 
 class TestCodeReviewComment(TestCaseWithFactory):
     """Test that comments are generated as expected."""
@@ -245,26 +320,14 @@
         set_feature_flag(u'code.inline_diff_comments.enabled', u'enabled')
 
         comment = self.makeCommentWithInlineComments(
-            inline_comments={'2': u'Is this from Planet Earth\xa9 ?'})
+            inline_comments={'10': u'Are we in development\xa9 ?',
+                             '39': u'Is this development\xa9 ?'})
         mailer = CodeReviewCommentMailer.forCreation(comment)
         commenter = comment.branch_merge_proposal.registrant
         ctrl = mailer.generateEmail(
             commenter.preferredemail.email, commenter)
 
-        expected_lines = [
-            '',
-            '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'
-        ]
-        self.assertEqual(expected_lines, ctrl.body.splitlines()[1:10])
+        self.assertEqual(expected_lines, ctrl.body.splitlines())
 
     def test_generateEmailWithInlineComments_feature_disabled(self):
         """Inline comments are not considered if the flag is not enabled."""

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2014-06-20 10:53:51 +0000
+++ lib/lp/testing/factory.py	2014-06-30 23:09:28 +0000
@@ -348,20 +348,69 @@
 SPACE = ' '
 
 DIFF = """\
-=== 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
-@@ -121,6 +121,10 @@
-                 'Gur pbasyvpgf grkg qrfpevovat nal cngu be grkg pbasyvpgf.'),
-              ernqbayl=Gehr))
-
-+    unf_pbasyvpgf = Obby(
-+        gvgyr=_('Unf pbasyvpgf'), ernqbayl=Gehr,
-+        qrfpevcgvba=_('Gur cerivrjrq zretr cebqhprf pbasyvpgf.'))
-+
-     # Gur fpurzn sbe gur Ersrerapr trgf cngpurq va _fpurzn_pvephyne_vzcbegf.
-     oenapu_zretr_cebcbfny = rkcbegrq(
-         Ersrerapr(
+=== modified file 'charms/precise/python-django/config.yaml'
+--- charms/precise/python-django/config.yaml    2014-05-30 14:46:01 +0000
++++ charms/precise/python-django/config.yaml    2014-06-06 18:15:02 +0000
+@@ -67,6 +67,10 @@
+         type: string
+         default: '/srv/'
+         description: The root directory to checkout to.
++    instance_type:
++        type: string
++        default: 'development'
++        description: 'Instance type (development, staging, or production)'
+     application_path:
+         type: string
+         default: ''
+
+=== modified file 'charms/precise/python-django/hooks/hooks.py'
+--- charms/precise/python-django/hooks/hooks.py 2014-06-05 15:32:53 +0000
++++ charms/precise/python-django/hooks/hooks.py 2014-06-06 23:38:36 +0000
+@@ -788,7 +788,14 @@
+
+     if not config_data['python_path']:
+         relation_set({'python_path': install_root})
+-
++    if config_data['wsgi_log_file']:
++        relation_set(
++            {'wsgi_log_file': os.path.join(
++                django_logs_dir, config_data['wsgi_log_file'])})
++    if config_data['wsgi_access_logfile']:
++        relation_set(
++            {'wsgi_access_logfile': os.path.join(
++                django_logs_dir, config_data['wsgi_access_logfile'])})
+     open_port(config_data['port'])
+
+
+@@ -919,19 +926,21 @@
+ django_south = config_data['django_south']
+ if django_south:
+     CHARM_PACKAGES.append('python-django-south')
+-
++instance_type = config_data['instance_type']
+ unit_name = os.environ['JUJU_UNIT_NAME'].split('/')[0]
+ sanitized_unit_name = sanitize(unit_name)
+-vcs_clone_dir = os.path.join(install_root, sanitized_unit_name)
++base_dir = os.path.join(install_root, sanitized_unit_name)
++django_settings_modules = '.'.join([sanitized_unit_name, django_settings])
++django_settings_modules = django_settings  # andy hack
++django_run_dir = os.path.join(base_dir, instance_type + '-run')
++django_logs_dir = os.path.join(base_dir, instance_type + '-logs')
++if config_data['wsgi_log_file'] or config_data['wsgi_access_logfile']:
++    install_dir(django_logs_dir, owner=wsgi_user, group=wsgi_group, mode=0755)
++vcs_clone_dir = os.path.join(base_dir, instance_type)
+ if application_path:
+     working_dir = os.path.join(vcs_clone_dir, application_path)
+ else:
+     working_dir = vcs_clone_dir
+-
+-django_settings_modules = '.'.join([sanitized_unit_name, django_settings])
+-django_settings_modules = django_settings  # andy hack
+-django_run_dir = os.path.join(working_dir, 'run/')
+-django_logs_dir = os.path.join(working_dir, 'logs/')
+ settings_py_path = os.path.join(working_dir, 'settings.py')
+ urls_py_path = os.path.join(working_dir, 'urls.py')
+ settings_dir_path = os.path.join(
 """