launchpad-reviewers team mailing list archive

[Maximiliano Bertacchini <maximiliano.bertacchini@xxxxxxxxxxxxx>]

Maximiliano Bertacchini has proposed merging lp:~maxiberta/launchpad/googlesearchservice-improvements into lp:launchpad.

Commit message:
A few improvements on GoogleSearchService.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~maxiberta/launchpad/googlesearchservice-improvements/+merge/342146

A few improvements on GoogleSearchService.

- Raise ValueError instead of AssertionError.
- Use urllib.urlencode instead of custom code.
- Import HTTPServer from six.
- Use lp.services.osutils.remove_if_exists() instead of custom safe_unlink().
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~maxiberta/launchpad/googlesearchservice-improvements into lp:launchpad.

=== modified file 'lib/lp/services/sitesearch/__init__.py'
--- lib/lp/services/sitesearch/__init__.py	2018-03-16 14:02:16 +0000
+++ lib/lp/services/sitesearch/__init__.py	2018-03-26 21:22:51 +0000
@@ -224,12 +224,12 @@
     def _checkParameter(self, name, value, is_int=False):
         """Check that a parameter value is not None or an empty string."""
         if value in (None, ''):
-            raise AssertionError("Missing value for parameter '%s'." % name)
+            raise ValueError("Missing value for parameter '%s'." % name)
         if is_int:
             try:
                 int(value)
             except ValueError:
-                raise AssertionError(
+                raise ValueError(
                     "Value for parameter '%s' is not an int." % name)
 
     def create_search_url(self, terms, start=0):
@@ -237,16 +237,13 @@
         self._checkParameter('q', terms)
         self._checkParameter('start', start, is_int=True)
         self._checkParameter('cx', self.client_id)
-        safe_terms = urllib.quote_plus(terms.encode('utf8'))
         search_params = dict(self._default_values)
-        search_params['q'] = safe_terms
+        search_params['q'] = terms.encode('utf8')
         search_params['start'] = start
         search_params['cx'] = self.client_id
-        search_param_list = []
-        for name in sorted(search_params):
-            value = search_params[name]
-            search_param_list.append('%s=%s' % (name, value))
-        query_string = '&'.join(search_param_list)
+        search_param_list = [
+            (name, value) for name, value in sorted(search_params.items())]
+        query_string = urllib.urlencode(search_param_list)
         return self.site + '?' + query_string
 
     def _getElementsByAttributeValue(self, doc, path, name, value):

=== modified file 'lib/lp/services/sitesearch/doc/google-searchservice.txt'
--- lib/lp/services/sitesearch/doc/google-searchservice.txt	2018-03-16 14:02:16 +0000
+++ lib/lp/services/sitesearch/doc/google-searchservice.txt	2018-03-26 21:22:51 +0000
@@ -208,17 +208,17 @@
     >>> google_search.create_search_url('')
     Traceback (most recent call last):
      ...
-    AssertionError: Missing value for parameter 'q'.
+    ValueError: Missing value for parameter 'q'.
 
     >>> google_search.create_search_url(None)
     Traceback (most recent call last):
      ...
-    AssertionError: Missing value for parameter 'q'.
+    ValueError: Missing value for parameter 'q'.
 
     >>> google_search.create_search_url('bugs', start='true')
     Traceback (most recent call last):
      ...
-    AssertionError: Value for parameter 'start' is not an int.
+    ValueError: Value for parameter 'start' is not an int.
 
 The term parameter in this example can be defined by passing the term
 argument to the method. The argument is url encoded and used as the

=== modified file 'lib/lp/services/sitesearch/googletestservice.py'
--- lib/lp/services/sitesearch/googletestservice.py	2018-03-16 14:31:03 +0000
+++ lib/lp/services/sitesearch/googletestservice.py	2018-03-26 21:22:51 +0000
@@ -9,10 +9,6 @@
 """
 
 
-from BaseHTTPServer import (
-    BaseHTTPRequestHandler,
-    HTTPServer,
-    )
 import errno
 import logging
 import os
@@ -21,8 +17,16 @@
 import subprocess
 import time
 
+from six.moves.BaseHTTPServer import (
+    BaseHTTPRequestHandler,
+    HTTPServer,
+    )
+
 from lp.services.config import config
-from lp.services.osutils import ensure_directory_exists
+from lp.services.osutils import (
+    ensure_directory_exists,
+    remove_if_exists,
+    )
 from lp.services.pidfile import (
     get_pid,
     make_pidfile,
@@ -220,7 +224,7 @@
     except ValueError:
         # The file contained a mangled and invalid PID number, so we should
         # clean the file up.
-        safe_unlink(pidfile_path(service_name))
+        remove_if_exists(pidfile_path(service_name))
     else:
         if pid is not None:
             try:
@@ -236,20 +240,11 @@
                     # is probably stale, so we'll remove it to prevent trash
                     # from lying around in the test environment.
                     # See bug #237086.
-                    safe_unlink(pidfile_path(service_name))
+                    remove_if_exists(pidfile_path(service_name))
                 else:
                     raise
 
 
-def safe_unlink(filepath):
-    """Unlink a file, but don't raise an error if the file is missing."""
-    try:
-        os.unlink(filepath)
-    except os.error as err:
-        if err.errno != errno.ENOENT:
-            raise
-
-
 def main():
     """Run the HTTP server."""
     # Redirect our service output to a log file.