Launchpad itself

Merge lp:~thumper/launchpad/diff-tales into lp:launchpad

  1. diff-tales
  2. Merge into devel
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 11007
Proposed branch: lp:~thumper/launchpad/diff-tales
Merge into: lp:launchpad
Diff against target: 504 lines (+241/-207)
3 files modified
lib/canonical/launchpad/webapp/tests/test_tales.py (+0/-205)
lib/lp/app/browser/stringformatter.py (+10/-2)
lib/lp/app/browser/tests/test_stringformatter.py (+231/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/diff-tales
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
Review via email: mp+27469@code.launchpad.net

Commit message

Fix the incorrect identification of the removal of lines starting with -- or the addition of lines starting with ++ in the string diff formatter.

Description of the change

This branch fixes two very related bugs:
  bug #553642: Incorrect display of diff lines starting with +++
  bug #561162: diff colorization fails with deleted SQL comments

This was due to the simplistic nature of the parser. I've added a very small amount of smarts to the parser so it only looks for the file headers directly after a filename.

I also moved the stringformatter TALES tests into lp.app.browser.tests now that the stringformatter is in lp.app.browser.

This does however obscure the actual test for this:
  TestDiffFormatter.test_cssClasses now has two lines to format, one starting with several ---- and the other with +++++

To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

All the new code looks good.

I find it worrying we are munging HTML rather than munging strings before we generate the HTML (break_long_words), and that the method name doesn't indicate it accepts HTML input rather than plain text. But this is old code.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/webapp/tests/test_tales.py'
2--- lib/canonical/launchpad/webapp/tests/test_tales.py 2010-05-21 02:33:14 +0000
3+++ lib/canonical/launchpad/webapp/tests/test_tales.py 2010-06-14 04:41:26 +0000
4@@ -3,19 +3,10 @@
5
6 """tales.py doctests."""
7
8-from textwrap import dedent
9 import unittest
10
11-from zope.component import getUtility
12 from zope.testing.doctestunit import DocTestSuite
13
14-from canonical.config import config
15-from canonical.launchpad.testing.pages import find_tags_by_class
16-from canonical.launchpad.webapp.interfaces import ILaunchBag
17-from canonical.testing import DatabaseFunctionalLayer
18-from lp.app.browser.stringformatter import FormattersAPI
19-from lp.testing import TestCase
20-
21
22 def test_requestapi():
23 """
24@@ -106,202 +97,6 @@
25
26 """
27
28-def test_split_paragraphs():
29- r"""
30- The split_paragraphs() method is used to split a block of text
31- into paragraphs, which are separated by one or more blank lines.
32- Paragraphs are yielded as a list of lines in the paragraph.
33-
34- >>> from lp.app.browser.stringformatter import split_paragraphs
35- >>> for paragraph in split_paragraphs('\na\nb\n\nc\nd\n\n\n'):
36- ... print paragraph
37- ['a', 'b']
38- ['c', 'd']
39- """
40-
41-def test_re_substitute():
42- """
43- When formatting text, we want to replace portions with links.
44- re.sub() works fairly well for this, but doesn't give us much
45- control over the non-matched text. The re_substitute() function
46- lets us do that.
47-
48- >>> import re
49- >>> from lp.app.browser.stringformatter import re_substitute
50-
51- >>> def match_func(match):
52- ... return '[%s]' % match.group()
53- >>> def nomatch_func(text):
54- ... return '{%s}' % text
55-
56- >>> pat = re.compile('a{2,6}')
57- >>> print re_substitute(pat, match_func, nomatch_func,
58- ... 'bbaaaabbbbaaaaaaa aaaaaaaab')
59- {bb}[aaaa]{bbbb}[aaaaaa]{a }[aaaaaa][aa]{b}
60- """
61-
62-def test_add_word_breaks():
63- """
64- Long words can cause page layout problems, so we insert manual
65- word breaks into long words. Breaks are added at least once every
66- 15 characters, but will break on as little as 7 characters if
67- there is a suitable non-alphanumeric character to break after.
68-
69- >>> from lp.app.browser.stringformatter import add_word_breaks
70-
71- >>> print add_word_breaks('abcdefghijklmnop')
72- abcdefghijklmno<wbr></wbr>p
73-
74- >>> print add_word_breaks('abcdef/ghijklmnop')
75- abcdef/<wbr></wbr>ghijklmnop
76-
77- >>> print add_word_breaks('ab/cdefghijklmnop')
78- ab/cdefghijklmn<wbr></wbr>op
79-
80- The string can contain HTML entities, which do not get split:
81-
82- >>> print add_word_breaks('abcdef&anentity;hijklmnop')
83- abcdef&anentity;<wbr></wbr>hijklmnop
84- """
85-
86-def test_break_long_words():
87- """
88- If we have a long HTML string, break_long_words() can be used to
89- add word breaks to the long words. It will not add breaks inside HTML
90- tags. Only words longer than 20 characters will have breaks added.
91-
92- >>> from lp.app.browser.stringformatter import break_long_words
93-
94- >>> print break_long_words('1234567890123456')
95- 1234567890123456
96-
97- >>> print break_long_words('12345678901234567890')
98- 123456789012345<wbr></wbr>67890
99-
100- >>> print break_long_words('<tag a12345678901234567890="foo"></tag>')
101- <tag a12345678901234567890="foo"></tag>
102-
103- >>> print break_long_words('12345678901234567890 1234567890.1234567890')
104- 123456789012345<wbr></wbr>67890 1234567890.<wbr></wbr>1234567890
105-
106- >>> print break_long_words('1234567890&abcdefghi;123')
107- 1234567890&abcdefghi;123
108-
109- >>> print break_long_words('<tag>1234567890123456</tag>')
110- <tag>1234567890123456</tag>
111- """
112-
113-
114-class TestDiffFormatter(TestCase):
115- """Test the string formtter fmt:diff."""
116- layer = DatabaseFunctionalLayer
117-
118- def test_emptyString(self):
119- # An empty string gives an empty string.
120- self.assertEqual(
121- '', FormattersAPI('').format_diff())
122-
123- def test_almostEmptyString(self):
124- # White space doesn't count as empty, and is formtted.
125- self.assertEqual(
126- '<table class="diff"><tr><td class="line-no">1</td>'
127- '<td class="text"> </td></tr></table>',
128- FormattersAPI(' ').format_diff())
129-
130- def test_format_unicode(self):
131- # Sometimes the strings contain unicode, those should work too.
132- self.assertEqual(
133- u'<table class="diff"><tr><td class="line-no">1</td>'
134- u'<td class="text">Unicode \u1010</td></tr></table>',
135- FormattersAPI(u'Unicode \u1010').format_diff())
136-
137- def test_cssClasses(self):
138- # Different parts of the diff have different css classes.
139- diff = dedent('''\
140- === modified file 'tales.py'
141- --- tales.py
142- +++ tales.py
143- @@ -2435,6 +2435,8 @@
144- def format_diff(self):
145- - removed this line
146- + added this line
147- ########
148- # A merge directive comment.
149- ''')
150- html = FormattersAPI(diff).format_diff()
151- line_numbers = find_tags_by_class(html, 'line-no')
152- self.assertEqual(
153- ['1','2','3','4','5','6','7','8','9'],
154- [tag.renderContents() for tag in line_numbers])
155- text = find_tags_by_class(html, 'text')
156- self.assertEqual(
157- ['diff-file text',
158- 'diff-header text',
159- 'diff-header text',
160- 'diff-chunk text',
161- 'text',
162- 'diff-removed text',
163- 'diff-added text',
164- 'diff-comment text',
165- 'diff-comment text'],
166- [str(tag['class']) for tag in text])
167-
168- def test_config_value_limits_line_count(self):
169- # The config.diff.max_line_format contains the maximum number of lines
170- # to format.
171- diff = dedent('''\
172- === modified file 'tales.py'
173- --- tales.py
174- +++ tales.py
175- @@ -2435,6 +2435,8 @@
176- def format_diff(self):
177- - removed this line
178- + added this line
179- ########
180- # A merge directive comment.
181- ''')
182- self.pushConfig("diff", max_format_lines=3)
183- html = FormattersAPI(diff).format_diff()
184- line_count = html.count('<td class="line-no">')
185- self.assertEqual(3, line_count)
186-
187-
188-class TestOOPSFormatter(TestCase):
189- """A test case for the oops_id() string formatter."""
190-
191- layer = DatabaseFunctionalLayer
192-
193- def test_doesnt_linkify_for_non_developers(self):
194- # OOPS IDs won't be linkified for non-developers.
195- oops_id = 'OOPS-12345TEST'
196- formatter = FormattersAPI(oops_id)
197- formatted_string = formatter.oops_id()
198-
199- self.assertEqual(
200- oops_id, formatted_string,
201- "Formatted string should be '%s', was '%s'" % (
202- oops_id, formatted_string))
203-
204- def _setDeveloper(self):
205- """Override ILaunchBag.developer for testing purposes."""
206- launch_bag = getUtility(ILaunchBag)
207- launch_bag.setDeveloper(True)
208-
209- def test_linkifies_for_developers(self):
210- # OOPS IDs will be linkified for Launchpad developers.
211- oops_id = 'OOPS-12345TEST'
212- formatter = FormattersAPI(oops_id)
213-
214- self._setDeveloper()
215- formatted_string = formatter.oops_id()
216-
217- expected_string = '<a href="%s">%s</a>' % (
218- config.launchpad.oops_root_url + oops_id, oops_id)
219-
220- self.assertEqual(
221- expected_string, formatted_string,
222- "Formatted string should be '%s', was '%s'" % (
223- expected_string, formatted_string))
224
225
226 def test_suite():
227
228=== modified file 'lib/lp/app/browser/stringformatter.py'
229--- lib/lp/app/browser/stringformatter.py 2010-05-21 02:33:14 +0000
230+++ lib/lp/app/browser/stringformatter.py 2010-06-14 04:41:26 +0000
231@@ -722,27 +722,35 @@
232 result = ['<table class="diff">']
233
234 max_format_lines = config.diff.max_format_lines
235+ header_next = False
236 for row, line in enumerate(text.splitlines()[:max_format_lines]):
237 result.append('<tr>')
238 result.append('<td class="line-no">%s</td>' % (row+1))
239 if line.startswith('==='):
240 css_class = 'diff-file text'
241- elif (line.startswith('+++') or
242- line.startswith('---')):
243+ header_next = True
244+ elif (header_next and
245+ (line.startswith('+++') or
246+ line.startswith('---'))):
247 css_class = 'diff-header text'
248 elif line.startswith('@@'):
249 css_class = 'diff-chunk text'
250+ header_next = False
251 elif line.startswith('+'):
252 css_class = 'diff-added text'
253+ header_next = False
254 elif line.startswith('-'):
255 css_class = 'diff-removed text'
256+ header_next = False
257 elif line.startswith('#'):
258 # This doesn't occur in normal unified diffs, but does
259 # appear in merge directives, which use text/x-diff or
260 # text/x-patch.
261 css_class = 'diff-comment text'
262+ header_next = False
263 else:
264 css_class = 'text'
265+ header_next = False
266 result.append(
267 '<td class="%s">%s</td>' % (css_class, escape(line)))
268 result.append('</tr>')
269
270=== added file 'lib/lp/app/browser/tests/test_stringformatter.py'
271--- lib/lp/app/browser/tests/test_stringformatter.py 1970-01-01 00:00:00 +0000
272+++ lib/lp/app/browser/tests/test_stringformatter.py 2010-06-14 04:41:26 +0000
273@@ -0,0 +1,231 @@
274+# Copyright 2010 Canonical Ltd. This software is licensed under the
275+# GNU Affero General Public License version 3 (see the file LICENSE).
276+
277+"""Unit tests for the string TALES formatter."""
278+
279+__metaclass__ = type
280+
281+from textwrap import dedent
282+import unittest
283+
284+from zope.component import getUtility
285+from zope.testing.doctestunit import DocTestSuite
286+
287+from canonical.config import config
288+from canonical.launchpad.testing.pages import find_tags_by_class
289+from canonical.launchpad.webapp.interfaces import ILaunchBag
290+from canonical.testing import DatabaseFunctionalLayer
291+from lp.app.browser.stringformatter import FormattersAPI
292+from lp.testing import TestCase
293+
294+
295+def test_split_paragraphs():
296+ r"""
297+ The split_paragraphs() method is used to split a block of text
298+ into paragraphs, which are separated by one or more blank lines.
299+ Paragraphs are yielded as a list of lines in the paragraph.
300+
301+ >>> from lp.app.browser.stringformatter import split_paragraphs
302+ >>> for paragraph in split_paragraphs('\na\nb\n\nc\nd\n\n\n'):
303+ ... print paragraph
304+ ['a', 'b']
305+ ['c', 'd']
306+ """
307+
308+
309+def test_re_substitute():
310+ """
311+ When formatting text, we want to replace portions with links.
312+ re.sub() works fairly well for this, but doesn't give us much
313+ control over the non-matched text. The re_substitute() function
314+ lets us do that.
315+
316+ >>> import re
317+ >>> from lp.app.browser.stringformatter import re_substitute
318+
319+ >>> def match_func(match):
320+ ... return '[%s]' % match.group()
321+ >>> def nomatch_func(text):
322+ ... return '{%s}' % text
323+
324+ >>> pat = re.compile('a{2,6}')
325+ >>> print re_substitute(pat, match_func, nomatch_func,
326+ ... 'bbaaaabbbbaaaaaaa aaaaaaaab')
327+ {bb}[aaaa]{bbbb}[aaaaaa]{a }[aaaaaa][aa]{b}
328+ """
329+
330+
331+def test_add_word_breaks():
332+ """
333+ Long words can cause page layout problems, so we insert manual
334+ word breaks into long words. Breaks are added at least once every
335+ 15 characters, but will break on as little as 7 characters if
336+ there is a suitable non-alphanumeric character to break after.
337+
338+ >>> from lp.app.browser.stringformatter import add_word_breaks
339+
340+ >>> print add_word_breaks('abcdefghijklmnop')
341+ abcdefghijklmno<wbr></wbr>p
342+
343+ >>> print add_word_breaks('abcdef/ghijklmnop')
344+ abcdef/<wbr></wbr>ghijklmnop
345+
346+ >>> print add_word_breaks('ab/cdefghijklmnop')
347+ ab/cdefghijklmn<wbr></wbr>op
348+
349+ The string can contain HTML entities, which do not get split:
350+
351+ >>> print add_word_breaks('abcdef&anentity;hijklmnop')
352+ abcdef&anentity;<wbr></wbr>hijklmnop
353+ """
354+
355+
356+def test_break_long_words():
357+ """
358+ If we have a long HTML string, break_long_words() can be used to
359+ add word breaks to the long words. It will not add breaks inside HTML
360+ tags. Only words longer than 20 characters will have breaks added.
361+
362+ >>> from lp.app.browser.stringformatter import break_long_words
363+
364+ >>> print break_long_words('1234567890123456')
365+ 1234567890123456
366+
367+ >>> print break_long_words('12345678901234567890')
368+ 123456789012345<wbr></wbr>67890
369+
370+ >>> print break_long_words('<tag a12345678901234567890="foo"></tag>')
371+ <tag a12345678901234567890="foo"></tag>
372+
373+ >>> print break_long_words('12345678901234567890 1234567890.1234567890')
374+ 123456789012345<wbr></wbr>67890 1234567890.<wbr></wbr>1234567890
375+
376+ >>> print break_long_words('1234567890&abcdefghi;123')
377+ 1234567890&abcdefghi;123
378+
379+ >>> print break_long_words('<tag>1234567890123456</tag>')
380+ <tag>1234567890123456</tag>
381+ """
382+
383+
384+class TestDiffFormatter(TestCase):
385+ """Test the string formtter fmt:diff."""
386+ layer = DatabaseFunctionalLayer
387+
388+ def test_emptyString(self):
389+ # An empty string gives an empty string.
390+ self.assertEqual(
391+ '', FormattersAPI('').format_diff())
392+
393+ def test_almostEmptyString(self):
394+ # White space doesn't count as empty, and is formtted.
395+ self.assertEqual(
396+ '<table class="diff"><tr><td class="line-no">1</td>'
397+ '<td class="text"> </td></tr></table>',
398+ FormattersAPI(' ').format_diff())
399+
400+ def test_format_unicode(self):
401+ # Sometimes the strings contain unicode, those should work too.
402+ self.assertEqual(
403+ u'<table class="diff"><tr><td class="line-no">1</td>'
404+ u'<td class="text">Unicode \u1010</td></tr></table>',
405+ FormattersAPI(u'Unicode \u1010').format_diff())
406+
407+ def test_cssClasses(self):
408+ # Different parts of the diff have different css classes.
409+ diff = dedent('''\
410+ === modified file 'tales.py'
411+ --- tales.py
412+ +++ tales.py
413+ @@ -2435,6 +2435,8 @@
414+ def format_diff(self):
415+ - removed this line
416+ + added this line
417+ -------- a sql style comment
418+ ++++++++ a line of pluses
419+ ########
420+ # A merge directive comment.
421+ ''')
422+ html = FormattersAPI(diff).format_diff()
423+ line_numbers = find_tags_by_class(html, 'line-no')
424+ self.assertEqual(
425+ ['1','2','3','4','5','6','7','8','9', '10', '11'],
426+ [tag.renderContents() for tag in line_numbers])
427+ text = find_tags_by_class(html, 'text')
428+ self.assertEqual(
429+ ['diff-file text',
430+ 'diff-header text',
431+ 'diff-header text',
432+ 'diff-chunk text',
433+ 'text',
434+ 'diff-removed text',
435+ 'diff-added text',
436+ 'diff-removed text',
437+ 'diff-added text',
438+ 'diff-comment text',
439+ 'diff-comment text'],
440+ [str(tag['class']) for tag in text])
441+
442+ def test_config_value_limits_line_count(self):
443+ # The config.diff.max_line_format contains the maximum number of lines
444+ # to format.
445+ diff = dedent('''\
446+ === modified file 'tales.py'
447+ --- tales.py
448+ +++ tales.py
449+ @@ -2435,6 +2435,8 @@
450+ def format_diff(self):
451+ - removed this line
452+ + added this line
453+ ########
454+ # A merge directive comment.
455+ ''')
456+ self.pushConfig("diff", max_format_lines=3)
457+ html = FormattersAPI(diff).format_diff()
458+ line_count = html.count('<td class="line-no">')
459+ self.assertEqual(3, line_count)
460+
461+
462+class TestOOPSFormatter(TestCase):
463+ """A test case for the oops_id() string formatter."""
464+
465+ layer = DatabaseFunctionalLayer
466+
467+ def test_doesnt_linkify_for_non_developers(self):
468+ # OOPS IDs won't be linkified for non-developers.
469+ oops_id = 'OOPS-12345TEST'
470+ formatter = FormattersAPI(oops_id)
471+ formatted_string = formatter.oops_id()
472+
473+ self.assertEqual(
474+ oops_id, formatted_string,
475+ "Formatted string should be '%s', was '%s'" % (
476+ oops_id, formatted_string))
477+
478+ def _setDeveloper(self):
479+ """Override ILaunchBag.developer for testing purposes."""
480+ launch_bag = getUtility(ILaunchBag)
481+ launch_bag.setDeveloper(True)
482+
483+ def test_linkifies_for_developers(self):
484+ # OOPS IDs will be linkified for Launchpad developers.
485+ oops_id = 'OOPS-12345TEST'
486+ formatter = FormattersAPI(oops_id)
487+
488+ self._setDeveloper()
489+ formatted_string = formatter.oops_id()
490+
491+ expected_string = '<a href="%s">%s</a>' % (
492+ config.launchpad.oops_root_url + oops_id, oops_id)
493+
494+ self.assertEqual(
495+ expected_string, formatted_string,
496+ "Formatted string should be '%s', was '%s'" % (
497+ expected_string, formatted_string))
498+
499+
500+def test_suite():
501+ suite = unittest.TestSuite()
502+ suite.addTests(DocTestSuite())
503+ suite.addTests(unittest.TestLoader().loadTestsFromName(__name__))
504+ return suite