launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



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	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'))

Use splitlines() here and below for consistency with the existing code.

> +        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 works well for simple patches, but will parse_patches always return every line as being part of a hunk? If not, the line count might drift. I'd like to see some tests including more complex diffs. It might also be worth checking that all of the comments have been consumed, so we'll know if we've missed a case.

> +                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."
> +]
> +

This is getting pretty long and ugly. I'd use a smaller diff (but still with several hunks) and include the expected text in a dedent()ed triple-quoted string.

>  
>  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(

I'd continue to use a relatively compact ROT13d Launchpad diff to minimise the impact on the tree's greppability.

>  """
>  
>  
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/short-ic-emails/+merge/225095
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.