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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
