Merge lp:~andrea.corbellini/launchpad/blueprint-whiteboard-diffs into lp:launchpad/db-devel

[Andrea Corbellini]

[Andrea Corbellini (andrea.corbellini)]

When the whiteboard of a blueprint is edited, show the differences in the e-mail, not just the new whiteboard.

[Abel Deuring (adeuring)]

Hi Andrea,

> When the whiteboard of a blueprint is edited, show the differences
> in the e-mail, not just the new whiteboard.

This very nice improvement of the blueprint notifications. However,
I have two remarks:

>
>
>
> === modified file 'lib/canonical/launchpad/mailnotification.py'
> --- lib/canonical/launchpad/mailnotification.py 2009-07-17 18:46:25 +0000
> +++ lib/canonical/launchpad/mailnotification.py 2009-07-23 17:04:53 +0000
> @@ -991,9 +991,11 @@
> if spec_delta.whiteboard is not None:
> if info_lines:
> info_lines.append('')
> - info_lines.append('Whiteboard changed to:')
> - info_lines.append('')
> - info_lines.append(mail_wrapper.format(spec_delta.whiteboard))
> + whiteboard_delta = spec_delta.whiteboard
> + whiteboard_diff = get_unified_diff(
> + whiteboard_delta['old'], whiteboard_delta['new'], 72)
> + info_lines.append('Whiteboard changed:')
> + info_lines.append(whiteboard_diff)

So get_unified_diff() limits the line length to 72 characters. This is
reasonable when you build diffs for source code, where the line length
is not too large (at least for "sane looking" source code). But a line
of a whiteboard (in the sense: "the text chuncks separated by \n") can
be longer: an entire paragraph, having a length of, let's say, 500.

I wonder if you should first line-wrap the old text and the new text,
and then call get_uniified_diff() for these line-wrapped versions..

What do you think?

>
> if not info_lines:
> # The specification was modified, but we don't yet support
>
> === modified file 'lib/lp/blueprints/model/specification.py'
> --- lib/lp/blueprints/model/specification.py 2009-07-17 00:26:05 +0000
> +++ lib/lp/blueprints/model/specification.py 2009-07-23 17:04:53 +0000
> @@ -446,11 +446,12 @@
> def getDelta(self, old_spec, user):
> """See ISpecification."""
> delta = ObjectDelta(old_spec, self)
> - delta.recordNewValues(("title", "summary", "whiteboard",
> + delta.recordNewValues(("title", "summary",
> "specurl", "productseries",
> "distroseries", "milestone"))
> delta.recordNewAndOld(("name", "priority", "definition_status",
> - "target", "approver", "assignee", "drafter"))
> + "target", "approver", "assignee", "drafter",
> + "whiteboard"))
> delta.recordListAddedAndRemoved("bugs",
> "bugs_linked",
> "bugs_unlinked")
>
>

We aim to test all our code, so a test for your changes is missing ;)
And actually, there is a test:
lib/lp/blueprints/doc/specification-notifications.txt

If you run

  ./bin/test --test=specification-notifications.txt

you'll notice that your change will break this test in lines 163 and 204.
Could you please update this test?

Abel

review: Needs Fixing

[Björn Tillenius (bjornt)]

On Fri, Jul 24, 2009 at 09:11:53AM -0000, Abel Deuring wrote:
> > === modified file 'lib/canonical/launchpad/mailnotification.py'
> > --- lib/canonical/launchpad/mailnotification.py 2009-07-17 18:46:25 +0000
> > +++ lib/canonical/launchpad/mailnotification.py 2009-07-23 17:04:53 +0000
> > @@ -991,9 +991,11 @@
> > if spec_delta.whiteboard is not None:
> > if info_lines:
> > info_lines.append('')
> > - info_lines.append('Whiteboard changed to:')
> > - info_lines.append('')
> > - info_lines.append(mail_wrapper.format(spec_delta.whiteboard))
> > + whiteboard_delta = spec_delta.whiteboard
> > + whiteboard_diff = get_unified_diff(
> > + whiteboard_delta['old'], whiteboard_delta['new'], 72)
> > + info_lines.append('Whiteboard changed:')
> > + info_lines.append(whiteboard_diff)
>
> So get_unified_diff() limits the line length to 72 characters. This is
> reasonable when you build diffs for source code, where the line length
> is not too large (at least for "sane looking" source code). But a line
> of a whiteboard (in the sense: "the text chuncks separated by \n") can
> be longer: an entire paragraph, having a length of, let's say, 500.
>
> I wonder if you should first line-wrap the old text and the new text,
> and then call get_uniified_diff() for these line-wrapped versions..

The text_width argument to get_uniified_diff() is used to wrap long
lines first, to produce reasonably looking diffs. Well, at least it
should get used, looking at the implementation that argument is actually
ignored and 72 is hard-coded. Anyway, no need to wrap the text before
calling get_uniified_diff()>

    vote abstain

review: Abstain

[Abel Deuring (adeuring)]

On 24.07.2009 11:27, Björn Tillenius wrote:

>> I wonder if you should first line-wrap the old text and the new text,
>> and then call get_uniified_diff() for these line-wrapped versions..
>
> The text_width argument to get_uniified_diff() is used to wrap long
> lines first, to produce reasonably looking diffs. Well, at least it
> should get used, looking at the implementation that argument is actually
> ignored and 72 is hard-coded. Anyway, no need to wrap the text before
> calling get_uniified_diff()>

whoops, sure. Should have lookad at get_unified_diff() first...

Abel

[Andrea Corbellini (andrea.corbellini)]

Thank you for your reviews.

I've passed 72 to get_unified_diff() because I've seen 72 in the bugs code.
However, I'll fix the test (I didn't found it the first time).

[Abel Deuring (adeuring)]

Hi Andrea,

With the changed test, your branch looks good.

review: Approve

[Andrea Corbellini (andrea.corbellini)]

I've fixed all the tests in rev 8975.

[Abel Deuring (adeuring)]

On 25.07.2009 13:48, Andrea Corbellini wrote:
> I've fixed all the tests in rev 8975.

Looks good.

=== modified file 'lib/canonical/launchpad/mailnotification.py'
--- lib/canonical/launchpad/mailnotification.py	2009-07-17 18:46:25 +0000
+++ lib/canonical/launchpad/mailnotification.py	2009-07-23 17:04:53 +0000
@@ -991,9 +991,11 @@
     if spec_delta.whiteboard is not None:
         if info_lines:
             info_lines.append('')
-        info_lines.append('Whiteboard changed to:')
-        info_lines.append('')
-        info_lines.append(mail_wrapper.format(spec_delta.whiteboard))
+        whiteboard_delta = spec_delta.whiteboard
+        whiteboard_diff = get_unified_diff(
+            whiteboard_delta['old'], whiteboard_delta['new'], 72)
+        info_lines.append('Whiteboard changed:')
+        info_lines.append(whiteboard_diff)
 
     if not info_lines:
         # The specification was modified, but we don't yet support

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2009-07-17 00:26:05 +0000
+++ lib/lp/blueprints/model/specification.py	2009-07-23 17:04:53 +0000
@@ -446,11 +446,12 @@
     def getDelta(self, old_spec, user):
         """See ISpecification."""
         delta = ObjectDelta(old_spec, self)
-        delta.recordNewValues(("title", "summary", "whiteboard",
+        delta.recordNewValues(("title", "summary",
                                "specurl", "productseries",
                                "distroseries", "milestone"))
         delta.recordNewAndOld(("name", "priority", "definition_status",
-                               "target", "approver", "assignee", "drafter"))
+                               "target", "approver", "assignee", "drafter",
+                               "whiteboard"))
         delta.recordListAddedAndRemoved("bugs",
                                         "bugs_linked",
                                         "bugs_unlinked")