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
=== modified file 'lib/canonical/launchpad/webapp/tests/test_tales.py'
--- lib/canonical/launchpad/webapp/tests/test_tales.py 2010-05-21 02:33:14 +0000
+++ lib/canonical/launchpad/webapp/tests/test_tales.py 2010-06-14 04:41:26 +0000
@@ -3,19 +3,10 @@
33
4"""tales.py doctests."""4"""tales.py doctests."""
55
6from textwrap import dedent
7import unittest6import unittest
87
9from zope.component import getUtility
10from zope.testing.doctestunit import DocTestSuite8from zope.testing.doctestunit import DocTestSuite
119
12from canonical.config import config
13from canonical.launchpad.testing.pages import find_tags_by_class
14from canonical.launchpad.webapp.interfaces import ILaunchBag
15from canonical.testing import DatabaseFunctionalLayer
16from lp.app.browser.stringformatter import FormattersAPI
17from lp.testing import TestCase
18
1910
20def test_requestapi():11def test_requestapi():
21 """12 """
@@ -106,202 +97,6 @@
10697
107 """98 """
10899
109def test_split_paragraphs():
110 r"""
111 The split_paragraphs() method is used to split a block of text
112 into paragraphs, which are separated by one or more blank lines.
113 Paragraphs are yielded as a list of lines in the paragraph.
114
115 >>> from lp.app.browser.stringformatter import split_paragraphs
116 >>> for paragraph in split_paragraphs('\na\nb\n\nc\nd\n\n\n'):
117 ... print paragraph
118 ['a', 'b']
119 ['c', 'd']
120 """
121
122def test_re_substitute():
123 """
124 When formatting text, we want to replace portions with links.
125 re.sub() works fairly well for this, but doesn't give us much
126 control over the non-matched text. The re_substitute() function
127 lets us do that.
128
129 >>> import re
130 >>> from lp.app.browser.stringformatter import re_substitute
131
132 >>> def match_func(match):
133 ... return '[%s]' % match.group()
134 >>> def nomatch_func(text):
135 ... return '{%s}' % text
136
137 >>> pat = re.compile('a{2,6}')
138 >>> print re_substitute(pat, match_func, nomatch_func,
139 ... 'bbaaaabbbbaaaaaaa aaaaaaaab')
140 {bb}[aaaa]{bbbb}[aaaaaa]{a }[aaaaaa][aa]{b}
141 """
142
143def test_add_word_breaks():
144 """
145 Long words can cause page layout problems, so we insert manual
146 word breaks into long words. Breaks are added at least once every
147 15 characters, but will break on as little as 7 characters if
148 there is a suitable non-alphanumeric character to break after.
149
150 >>> from lp.app.browser.stringformatter import add_word_breaks
151
152 >>> print add_word_breaks('abcdefghijklmnop')
153 abcdefghijklmno<wbr></wbr>p
154
155 >>> print add_word_breaks('abcdef/ghijklmnop')
156 abcdef/<wbr></wbr>ghijklmnop
157
158 >>> print add_word_breaks('ab/cdefghijklmnop')
159 ab/cdefghijklmn<wbr></wbr>op
160
161 The string can contain HTML entities, which do not get split:
162
163 >>> print add_word_breaks('abcdef&anentity;hijklmnop')
164 abcdef&anentity;<wbr></wbr>hijklmnop
165 """
166
167def test_break_long_words():
168 """
169 If we have a long HTML string, break_long_words() can be used to
170 add word breaks to the long words. It will not add breaks inside HTML
171 tags. Only words longer than 20 characters will have breaks added.
172
173 >>> from lp.app.browser.stringformatter import break_long_words
174
175 >>> print break_long_words('1234567890123456')
176 1234567890123456
177
178 >>> print break_long_words('12345678901234567890')
179 123456789012345<wbr></wbr>67890
180
181 >>> print break_long_words('<tag a12345678901234567890="foo"></tag>')
182 <tag a12345678901234567890="foo"></tag>
183
184 >>> print break_long_words('12345678901234567890 1234567890.1234567890')
185 123456789012345<wbr></wbr>67890 1234567890.<wbr></wbr>1234567890
186
187 >>> print break_long_words('1234567890&abcdefghi;123')
188 1234567890&abcdefghi;123
189
190 >>> print break_long_words('<tag>1234567890123456</tag>')
191 <tag>1234567890123456</tag>
192 """
193
194
195class TestDiffFormatter(TestCase):
196 """Test the string formtter fmt:diff."""
197 layer = DatabaseFunctionalLayer
198
199 def test_emptyString(self):
200 # An empty string gives an empty string.
201 self.assertEqual(
202 '', FormattersAPI('').format_diff())
203
204 def test_almostEmptyString(self):
205 # White space doesn't count as empty, and is formtted.
206 self.assertEqual(
207 '<table class="diff"><tr><td class="line-no">1</td>'
208 '<td class="text"> </td></tr></table>',
209 FormattersAPI(' ').format_diff())
210
211 def test_format_unicode(self):
212 # Sometimes the strings contain unicode, those should work too.
213 self.assertEqual(
214 u'<table class="diff"><tr><td class="line-no">1</td>'
215 u'<td class="text">Unicode \u1010</td></tr></table>',
216 FormattersAPI(u'Unicode \u1010').format_diff())
217
218 def test_cssClasses(self):
219 # Different parts of the diff have different css classes.
220 diff = dedent('''\
221 === modified file 'tales.py'
222 --- tales.py
223 +++ tales.py
224 @@ -2435,6 +2435,8 @@
225 def format_diff(self):
226 - removed this line
227 + added this line
228 ########
229 # A merge directive comment.
230 ''')
231 html = FormattersAPI(diff).format_diff()
232 line_numbers = find_tags_by_class(html, 'line-no')
233 self.assertEqual(
234 ['1','2','3','4','5','6','7','8','9'],
235 [tag.renderContents() for tag in line_numbers])
236 text = find_tags_by_class(html, 'text')
237 self.assertEqual(
238 ['diff-file text',
239 'diff-header text',
240 'diff-header text',
241 'diff-chunk text',
242 'text',
243 'diff-removed text',
244 'diff-added text',
245 'diff-comment text',
246 'diff-comment text'],
247 [str(tag['class']) for tag in text])
248
249 def test_config_value_limits_line_count(self):
250 # The config.diff.max_line_format contains the maximum number of lines
251 # to format.
252 diff = dedent('''\
253 === modified file 'tales.py'
254 --- tales.py
255 +++ tales.py
256 @@ -2435,6 +2435,8 @@
257 def format_diff(self):
258 - removed this line
259 + added this line
260 ########
261 # A merge directive comment.
262 ''')
263 self.pushConfig("diff", max_format_lines=3)
264 html = FormattersAPI(diff).format_diff()
265 line_count = html.count('<td class="line-no">')
266 self.assertEqual(3, line_count)
267
268
269class TestOOPSFormatter(TestCase):
270 """A test case for the oops_id() string formatter."""
271
272 layer = DatabaseFunctionalLayer
273
274 def test_doesnt_linkify_for_non_developers(self):
275 # OOPS IDs won't be linkified for non-developers.
276 oops_id = 'OOPS-12345TEST'
277 formatter = FormattersAPI(oops_id)
278 formatted_string = formatter.oops_id()
279
280 self.assertEqual(
281 oops_id, formatted_string,
282 "Formatted string should be '%s', was '%s'" % (
283 oops_id, formatted_string))
284
285 def _setDeveloper(self):
286 """Override ILaunchBag.developer for testing purposes."""
287 launch_bag = getUtility(ILaunchBag)
288 launch_bag.setDeveloper(True)
289
290 def test_linkifies_for_developers(self):
291 # OOPS IDs will be linkified for Launchpad developers.
292 oops_id = 'OOPS-12345TEST'
293 formatter = FormattersAPI(oops_id)
294
295 self._setDeveloper()
296 formatted_string = formatter.oops_id()
297
298 expected_string = '<a href="%s">%s</a>' % (
299 config.launchpad.oops_root_url + oops_id, oops_id)
300
301 self.assertEqual(
302 expected_string, formatted_string,
303 "Formatted string should be '%s', was '%s'" % (
304 expected_string, formatted_string))
305100
306101
307def test_suite():102def test_suite():
308103
=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py 2010-05-21 02:33:14 +0000
+++ lib/lp/app/browser/stringformatter.py 2010-06-14 04:41:26 +0000
@@ -722,27 +722,35 @@
722 result = ['<table class="diff">']722 result = ['<table class="diff">']
723723
724 max_format_lines = config.diff.max_format_lines724 max_format_lines = config.diff.max_format_lines
725 header_next = False
725 for row, line in enumerate(text.splitlines()[:max_format_lines]):726 for row, line in enumerate(text.splitlines()[:max_format_lines]):
726 result.append('<tr>')727 result.append('<tr>')
727 result.append('<td class="line-no">%s</td>' % (row+1))728 result.append('<td class="line-no">%s</td>' % (row+1))
728 if line.startswith('==='):729 if line.startswith('==='):
729 css_class = 'diff-file text'730 css_class = 'diff-file text'
730 elif (line.startswith('+++') or731 header_next = True
731 line.startswith('---')):732 elif (header_next and
733 (line.startswith('+++') or
734 line.startswith('---'))):
732 css_class = 'diff-header text'735 css_class = 'diff-header text'
733 elif line.startswith('@@'):736 elif line.startswith('@@'):
734 css_class = 'diff-chunk text'737 css_class = 'diff-chunk text'
738 header_next = False
735 elif line.startswith('+'):739 elif line.startswith('+'):
736 css_class = 'diff-added text'740 css_class = 'diff-added text'
741 header_next = False
737 elif line.startswith('-'):742 elif line.startswith('-'):
738 css_class = 'diff-removed text'743 css_class = 'diff-removed text'
744 header_next = False
739 elif line.startswith('#'):745 elif line.startswith('#'):
740 # This doesn't occur in normal unified diffs, but does746 # This doesn't occur in normal unified diffs, but does
741 # appear in merge directives, which use text/x-diff or747 # appear in merge directives, which use text/x-diff or
742 # text/x-patch.748 # text/x-patch.
743 css_class = 'diff-comment text'749 css_class = 'diff-comment text'
750 header_next = False
744 else:751 else:
745 css_class = 'text'752 css_class = 'text'
753 header_next = False
746 result.append(754 result.append(
747 '<td class="%s">%s</td>' % (css_class, escape(line)))755 '<td class="%s">%s</td>' % (css_class, escape(line)))
748 result.append('</tr>')756 result.append('</tr>')
749757
=== added file 'lib/lp/app/browser/tests/test_stringformatter.py'
--- lib/lp/app/browser/tests/test_stringformatter.py 1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/tests/test_stringformatter.py 2010-06-14 04:41:26 +0000
@@ -0,0 +1,231 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Unit tests for the string TALES formatter."""
5
6__metaclass__ = type
7
8from textwrap import dedent
9import unittest
10
11from zope.component import getUtility
12from zope.testing.doctestunit import DocTestSuite
13
14from canonical.config import config
15from canonical.launchpad.testing.pages import find_tags_by_class
16from canonical.launchpad.webapp.interfaces import ILaunchBag
17from canonical.testing import DatabaseFunctionalLayer
18from lp.app.browser.stringformatter import FormattersAPI
19from lp.testing import TestCase
20
21
22def test_split_paragraphs():
23 r"""
24 The split_paragraphs() method is used to split a block of text
25 into paragraphs, which are separated by one or more blank lines.
26 Paragraphs are yielded as a list of lines in the paragraph.
27
28 >>> from lp.app.browser.stringformatter import split_paragraphs
29 >>> for paragraph in split_paragraphs('\na\nb\n\nc\nd\n\n\n'):
30 ... print paragraph
31 ['a', 'b']
32 ['c', 'd']
33 """
34
35
36def test_re_substitute():
37 """
38 When formatting text, we want to replace portions with links.
39 re.sub() works fairly well for this, but doesn't give us much
40 control over the non-matched text. The re_substitute() function
41 lets us do that.
42
43 >>> import re
44 >>> from lp.app.browser.stringformatter import re_substitute
45
46 >>> def match_func(match):
47 ... return '[%s]' % match.group()
48 >>> def nomatch_func(text):
49 ... return '{%s}' % text
50
51 >>> pat = re.compile('a{2,6}')
52 >>> print re_substitute(pat, match_func, nomatch_func,
53 ... 'bbaaaabbbbaaaaaaa aaaaaaaab')
54 {bb}[aaaa]{bbbb}[aaaaaa]{a }[aaaaaa][aa]{b}
55 """
56
57
58def test_add_word_breaks():
59 """
60 Long words can cause page layout problems, so we insert manual
61 word breaks into long words. Breaks are added at least once every
62 15 characters, but will break on as little as 7 characters if
63 there is a suitable non-alphanumeric character to break after.
64
65 >>> from lp.app.browser.stringformatter import add_word_breaks
66
67 >>> print add_word_breaks('abcdefghijklmnop')
68 abcdefghijklmno<wbr></wbr>p
69
70 >>> print add_word_breaks('abcdef/ghijklmnop')
71 abcdef/<wbr></wbr>ghijklmnop
72
73 >>> print add_word_breaks('ab/cdefghijklmnop')
74 ab/cdefghijklmn<wbr></wbr>op
75
76 The string can contain HTML entities, which do not get split:
77
78 >>> print add_word_breaks('abcdef&anentity;hijklmnop')
79 abcdef&anentity;<wbr></wbr>hijklmnop
80 """
81
82
83def test_break_long_words():
84 """
85 If we have a long HTML string, break_long_words() can be used to
86 add word breaks to the long words. It will not add breaks inside HTML
87 tags. Only words longer than 20 characters will have breaks added.
88
89 >>> from lp.app.browser.stringformatter import break_long_words
90
91 >>> print break_long_words('1234567890123456')
92 1234567890123456
93
94 >>> print break_long_words('12345678901234567890')
95 123456789012345<wbr></wbr>67890
96
97 >>> print break_long_words('<tag a12345678901234567890="foo"></tag>')
98 <tag a12345678901234567890="foo"></tag>
99
100 >>> print break_long_words('12345678901234567890 1234567890.1234567890')
101 123456789012345<wbr></wbr>67890 1234567890.<wbr></wbr>1234567890
102
103 >>> print break_long_words('1234567890&abcdefghi;123')
104 1234567890&abcdefghi;123
105
106 >>> print break_long_words('<tag>1234567890123456</tag>')
107 <tag>1234567890123456</tag>
108 """
109
110
111class TestDiffFormatter(TestCase):
112 """Test the string formtter fmt:diff."""
113 layer = DatabaseFunctionalLayer
114
115 def test_emptyString(self):
116 # An empty string gives an empty string.
117 self.assertEqual(
118 '', FormattersAPI('').format_diff())
119
120 def test_almostEmptyString(self):
121 # White space doesn't count as empty, and is formtted.
122 self.assertEqual(
123 '<table class="diff"><tr><td class="line-no">1</td>'
124 '<td class="text"> </td></tr></table>',
125 FormattersAPI(' ').format_diff())
126
127 def test_format_unicode(self):
128 # Sometimes the strings contain unicode, those should work too.
129 self.assertEqual(
130 u'<table class="diff"><tr><td class="line-no">1</td>'
131 u'<td class="text">Unicode \u1010</td></tr></table>',
132 FormattersAPI(u'Unicode \u1010').format_diff())
133
134 def test_cssClasses(self):
135 # Different parts of the diff have different css classes.
136 diff = dedent('''\
137 === modified file 'tales.py'
138 --- tales.py
139 +++ tales.py
140 @@ -2435,6 +2435,8 @@
141 def format_diff(self):
142 - removed this line
143 + added this line
144 -------- a sql style comment
145 ++++++++ a line of pluses
146 ########
147 # A merge directive comment.
148 ''')
149 html = FormattersAPI(diff).format_diff()
150 line_numbers = find_tags_by_class(html, 'line-no')
151 self.assertEqual(
152 ['1','2','3','4','5','6','7','8','9', '10', '11'],
153 [tag.renderContents() for tag in line_numbers])
154 text = find_tags_by_class(html, 'text')
155 self.assertEqual(
156 ['diff-file text',
157 'diff-header text',
158 'diff-header text',
159 'diff-chunk text',
160 'text',
161 'diff-removed text',
162 'diff-added text',
163 'diff-removed text',
164 'diff-added text',
165 'diff-comment text',
166 'diff-comment text'],
167 [str(tag['class']) for tag in text])
168
169 def test_config_value_limits_line_count(self):
170 # The config.diff.max_line_format contains the maximum number of lines
171 # to format.
172 diff = dedent('''\
173 === modified file 'tales.py'
174 --- tales.py
175 +++ tales.py
176 @@ -2435,6 +2435,8 @@
177 def format_diff(self):
178 - removed this line
179 + added this line
180 ########
181 # A merge directive comment.
182 ''')
183 self.pushConfig("diff", max_format_lines=3)
184 html = FormattersAPI(diff).format_diff()
185 line_count = html.count('<td class="line-no">')
186 self.assertEqual(3, line_count)
187
188
189class TestOOPSFormatter(TestCase):
190 """A test case for the oops_id() string formatter."""
191
192 layer = DatabaseFunctionalLayer
193
194 def test_doesnt_linkify_for_non_developers(self):
195 # OOPS IDs won't be linkified for non-developers.
196 oops_id = 'OOPS-12345TEST'
197 formatter = FormattersAPI(oops_id)
198 formatted_string = formatter.oops_id()
199
200 self.assertEqual(
201 oops_id, formatted_string,
202 "Formatted string should be '%s', was '%s'" % (
203 oops_id, formatted_string))
204
205 def _setDeveloper(self):
206 """Override ILaunchBag.developer for testing purposes."""
207 launch_bag = getUtility(ILaunchBag)
208 launch_bag.setDeveloper(True)
209
210 def test_linkifies_for_developers(self):
211 # OOPS IDs will be linkified for Launchpad developers.
212 oops_id = 'OOPS-12345TEST'
213 formatter = FormattersAPI(oops_id)
214
215 self._setDeveloper()
216 formatted_string = formatter.oops_id()
217
218 expected_string = '<a href="%s">%s</a>' % (
219 config.launchpad.oops_root_url + oops_id, oops_id)
220
221 self.assertEqual(
222 expected_string, formatted_string,
223 "Formatted string should be '%s', was '%s'" % (
224 expected_string, formatted_string))
225
226
227def test_suite():
228 suite = unittest.TestSuite()
229 suite.addTests(DocTestSuite())
230 suite.addTests(unittest.TestLoader().loadTestsFromName(__name__))
231 return suite