Code review comment for lp:~andrea.corbellini/launchpad/blueprint-whiteboard-diffs

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