Merge lp:~jcsackett/launchpad/better-bug-linking-531889 into lp:launchpad

[j.c.sackett]

[r=leonardr][ui=none][bug=531889] Fixes linking conditions for bug references in bug comments.

Summary
=======

Bugs are automatically linked based on the appearance of certain strings (bug no <somenumber>, bug #<somenumber, &c). However, some of these combinations are linked even when they're the result of apparent typos. This branch updates the regex to avoid matching on these patterns.

Implementation
==============

The bug pattern group in the _re_linkify regex is slightly modified to deal with an appropriate amount of spacing and not match the the phrase "bug number" at the end of a sentence.

Tests
=====

bin/test -t LinkifyingBugs

Lint
====

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/browser/stringformatter.py
  lib/lp/app/browser/tests/test_stringformatter.py

./lib/lp/app/browser/stringformatter.py
     408: E501 line too long (88 characters)
     412: E501 line too long (86 characters)
     408: Line exceeds 78 characters.
     412: Line exceeds 78 characters.

E501s are from long sections of the primary linking regex, which may be even harder to read if those lines are wrapped.

[Leonard Richardson (leonardr)]

You left the ? in the "number?" part of your regular expressions, which will make "bug numbe 10" match when it shouldn't. Take that out and it looks good.

review: Approve

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2010-12-20 15:37:02 +0000
+++ lib/lp/app/browser/stringformatter.py	2010-12-21 16:19:53 +0000
@@ -457,11 +457,13 @@
          )
       ) |
       (?P<bug>
-        \bbug(?:[\s=-]|<br\s*/>)*(?:\#|report|number\.?|num\.?|no\.?)?(?:[\s=-]|<br\s*/>)*
+        \bbug(?:[\s=-]|<br\s*/>)*
+            (?:(?:(?:\#|report|number|num\.?|no\.?)?(?:[\s=-]|<br\s*/>)+)|
+            (?:(?:\s\#)?(?:[\s=-]|<br\s*/>)*))
         0*(?P<bugnum>\d+)
       ) |
       (?P<faq>
-        \bfaq(?:[\s=-]|<br\s*/>)*(?:\#|item|number\.?|num\.?|no\.?)?(?:[\s=-]|<br\s*/>)*
+        \bfaq(?:[\s=-]|<br\s*/>)*(?:\#|item|number?|num\.?|no\.?)?(?:[\s=-]|<br\s*/>)*
         0*(?P<faqnum>\d+)
       ) |
       (?P<oops>

=== modified file 'lib/lp/app/browser/tests/test_stringformatter.py'
--- lib/lp/app/browser/tests/test_stringformatter.py	2010-12-20 15:13:30 +0000
+++ lib/lp/app/browser/tests/test_stringformatter.py	2010-12-21 16:19:53 +0000
@@ -107,6 +107,40 @@
       <tag>1234567890123456</tag>
     """
 
+class TestLinkifyingBugs(TestCase):
+
+    def test_regular_bug_case_works(self):
+        test_strings = [
+            "bug 34434",
+            "bugnumber 34434",
+            "bug number 34434",
+            ]
+        expected_html = [
+            '<p><a href="/bugs/34434">bug 34434</a></p>',
+            '<p><a href="/bugs/34434">bugnumber 34434</a></p>',
+            '<p><a href="/bugs/34434">bug number 34434</a></p>',
+            ]
+        self.assertEqual(
+            expected_html,
+            [FormattersAPI(text).text_to_html() for text in test_strings])
+
+    def test_things_do_not_link_if_they_should_not(self):
+        test_strings = [
+            "bugnumber.4",
+            "bug number.4",
+            "bugno.4",
+            "bug no.4",
+            ]
+        expected_html = [
+            "<p>bugnumber.4</p>",
+            "<p>bug number.4</p>",
+            "<p>bugno.4</p>",
+            "<p>bug no.4</p>",
+            ]
+        self.assertEqual(
+            expected_html,
+            [FormattersAPI(text).text_to_html() for text in test_strings])
+
 
 class TestLinkifyingProtocols(TestCase):
 
@@ -271,7 +305,7 @@
         html = FormattersAPI(diff).format_diff()
         line_numbers = find_tags_by_class(html, 'line-no')
         self.assertEqual(
-            ['1','2','3','4','5','6','7','8','9', '10', '11'],
+            ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11'],
             [tag.renderContents() for tag in line_numbers])
         text = find_tags_by_class(html, 'text')
         self.assertEqual(

=== modified file 'lib/lp/app/doc/displaying-paragraphs-of-text.txt'
--- lib/lp/app/doc/displaying-paragraphs-of-text.txt	2010-10-19 03:28:24 +0000
+++ lib/lp/app/doc/displaying-paragraphs-of-text.txt	2010-12-21 16:19:53 +0000
@@ -229,13 +229,13 @@
     <a href="/bugs/123">bug    123</a><br />
     <a href="/bugs/123">bug #123</a><br />
     <a href="/bugs/123">bug number 123</a><br />
-    <a href="/bugs/123">bug number. 123</a><br />
+    bug number. 123<br />
     <a href="/bugs/123">bug num 123</a><br />
     <a href="/bugs/123">bug num. 123</a><br />
     <a href="/bugs/123">bug no 123</a><br />
     <a href="/bugs/123">bug report 123</a><br />
     <a href="/bugs/123">bug no. 123</a><br />
-    <a href="/bugs/123">bug#123</a><br />
+    bug#123<br />
     <a href="/bugs/123">bug-123</a><br />
     <a href="/bugs/123">bug-report-123</a><br />
     <a href="/bugs/123">bug=123</a><br />