launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/better-bug-linking-531889 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #531889 "bugnumber.<newline>2" is auto-linked
  https://bugs.launchpad.net/bugs/531889


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.
-- 
https://code.launchpad.net/~jcsackett/launchpad/better-bug-linking-531889/+merge/44098
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/better-bug-linking-531889 into lp:launchpad.

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2010-12-14 17:33:05 +0000
+++ lib/lp/app/browser/stringformatter.py	2010-12-17 21:10:14 +0000
@@ -405,11 +405,11 @@
          )
       ) |
       (?P<bug>
-        \bbug(?:[\s=-]|<br\s*/>)*(?:\#|report|number\.?|num\.?|no\.?)?(?:[\s=-]|<br\s*/>)*
+        \bbug(?:[\s=-]|<br\s*/>)*(?:\#|report|number?|num\.?|no\.?)?(?:[\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-14 19:56:40 +0000
+++ lib/lp/app/browser/tests/test_stringformatter.py	2010-12-17 21:10:14 +0000
@@ -107,6 +107,42 @@
       <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):
 
     def test_apt_is_linked(self):
@@ -124,7 +160,7 @@
         expected_html = (
             '<p>This becomes a link: '
             '<a rel="nofollow" '
-                'href="apt://some-package">apt://some-<wbr></wbr>package</a></p>')
+            'href="apt://some-package">apt://some-<wbr></wbr>package</a></p>')
         self.assertEqual(expected_html, html)
 
     def test_file_is_not_linked(self):
@@ -141,9 +177,10 @@
         expected_html = (
             "<p>This becomes a link: "
             '<a rel="nofollow" '
-            'href="data:text/plain,test">data:text/<wbr></wbr>plain,test</a></p>')
+            'href="data:text/plain,test">'
+            'data:text/<wbr></wbr>plain,test</a></p>')
         self.assertEqual(expected_html, html)
-            
+
 
 class TestDiffFormatter(TestCase):
     """Test the string formatter fmt:diff."""
@@ -186,7 +223,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(