rest: improve validation and argument processing in a few methods

ligo/gracedb/rest.py

@@ -905,38 +905,31 @@ class GraceDb(GsiRest):
             >>> r.status
             201
         """
-        errors = []
         if group not in self.groups:
-            errors += ["bad group"]
+            raise ValueError('bad group')
         if pipeline not in self.pipelines:
-            errors += ["bad pipeline"]
+            raise ValueError('bad pipeline')
         if search and search not in self.searches:
-            errors += ["bad search"]
+            raise ValueError('bad search')
         # Process offline arg
         if not isinstance(offline, bool):
-            errors += ["offline should be True or False"]
+            raise TypeError('offline parameter should be a bool')
         # Process label args - convert non-empty strings to list
         # to ensure consistent processing
-        if labels:
+        if labels is not None:
             if isinstance(labels, six.string_types):
                 # Convert to list
                 labels = [labels]
             elif isinstance(labels, list):
                 pass
             else:
-                # Raise exception instead of adding errors. The next for loop
-                # will break (before errors exception is raised) if labels
-                # is of the wrong type
                 raise TypeError("labels arg is {0}, should be str or list"
                                 .format(type(labels)))
             # Check labels against those in database
             for l in labels:
                 if l not in self.allowed_labels:
-                    raise NameError(("Label '{0}' does not exist in the "
+                    raise ValueError(("Label '{0}' does not exist in the "
                                      "database").format(l))
-        if errors:
-            # XXX Terrible error messages / weak exception type
-            raise Exception(str(errors))
         if filecontents is None:
             if filename == '-':
                 filename = 'initial.data'
@@ -1387,7 +1380,7 @@ class GraceDb(GsiRest):
         return self.get(self.templates['superevent-detail-template'].format(
             superevent_id=superevent_id))
 
-    def superevents(self, query='', orderby=[], count=None, columns=[],
+    def superevents(self, query='', orderby=None, count=None, columns=None,
                     max_results=None):
         """Search for superevents which match a query.
 
@@ -1425,7 +1418,7 @@ class GraceDb(GsiRest):
 
         # If orderby is a list (should be), convert it to a comma-separated
         # string (that's what the server expects)
-        if isinstance(orderby, list):
+        if isinstance(orderby, (list, tuple)):
             orderby = ",".join(orderby)
 
         # Get URI
@@ -1540,6 +1533,8 @@ class GraceDb(GsiRest):
                 template = self.templates['superevent-file-list-template']
         else:
             template = self.templates['files-template']
+            if not filename:
+                filename = ""
             uri_kwargs = {'graceid': object_id, 'filename': filename}
         uri = template.format(**uri_kwargs)
         return self.get(uri)
@@ -1547,7 +1542,8 @@ class GraceDb(GsiRest):
     def writeFile(self, object_id, filename, filecontents=None):
         print("WARNING: the writeFile() method is deprecated in favor "
               "of writeLog() and will be removed in a future release.")
-        return self.writeLog(object_id, "FILE UPLOAD", filename, filecontents)
+        return self.writeLog(object_id, "FILE UPLOAD", filename=filename,
+                             filecontents=filecontents)
 
     @event_or_superevent
     def logs(self, object_id, log_number=None, *args, **kwargs):
@@ -1645,9 +1641,25 @@ class GraceDb(GsiRest):
         """
         # Handle old usage of 'tagname' instead of 'tag_name'
         tagname = kwargs.pop('tagname', None)
-        if tagname is not None and tag_name == []:
+        if tagname is not None and not tag_name:
             tag_name = tagname
 
+        # Handle cases where tag_name is a string
+        if isinstance(tag_name, str):
+            tag_name = [tag_name]
+        elif isinstance(tag_name, (tuple, set)):
+            tag_name = list(tag_name)
+        elif tag_name is None:
+            tag_name = []
+
+        # Handle cases where displayName is a string
+        if isinstance(displayName, str):
+            displayName = [displayName]
+        elif isinstance(displayName, (tuple, set)):
+            displayName = list(displayName)
+        elif displayName is None:
+            displayName = []
+
         # Check displayName length - should be 0 or same as tag_name
         if (displayName and isinstance(tag_name, list)
             and len(displayName) != len(tag_name)):
@@ -1676,21 +1688,6 @@ class GraceDb(GsiRest):
                 filecontents = filecontents.read()
             files = [('upload', os.path.basename(filename), filecontents)]
 
-        # Handle cases where tag_name or displayName are strings
-        if isinstance(tag_name, str):
-            tag_name = [tag_name]
-        elif isinstance(tag_name, (tuple, set)):
-            tag_name = list(tag_name)
-        elif tag_name is None:
-            tag_name = []
-
-        if isinstance(displayName, str):
-            displayName = [displayName]
-        elif isinstance(displayName, (tuple, set)):
-            displayName = list(displayName)
-        elif displayName is None:
-            displayName = []
-
         # Set up body of request
         body = {
             'comment': message,
@@ -1710,7 +1707,7 @@ class GraceDb(GsiRest):
                     fields.append((k, v))
             body = fields
 
-        return self.post(uri, body, files=files)
+        return self.post(uri, body=body, files=files)
 
     @event_or_superevent
     def emobservations(self, object_id, emobservation_num=None, *args,
@@ -1813,7 +1810,9 @@ class GraceDb(GsiRest):
         """
         # Validate EM group
         if group not in self.em_groups:
-            raise ValueError("group must be one of %s" % self.em_groups)
+            err_msg = "group must be one of {groups}".format(
+                groups=", ".join(self.em_groups))
+            raise ValueError(err_msg)
 
         # Argument checking
         num_measurements = len(raList)
@@ -2227,7 +2226,7 @@ class GraceDb(GsiRest):
         voevent_type = self._getCode(voevent_type.lower(), self.voevent_types)
         if not voevent_type:
             raise ValueError("voevent_type must be one of: {0}".format(
-                list(six.itervalues(self.voevent_types))))
+                ", ".join(list(six.itervalues(self.voevent_types)))))
 
         # Require skymaps for 'update' and 'initial'
         if voevent_type == 'IN':
@@ -2460,8 +2459,10 @@ class GraceDb(GsiRest):
         else:
             raise NotImplementedError('Not yet implemented for events')
 
-        return self._signoff_helper(object_id, 'get', template, uri_kwargs,
-                                    signoff_type, instrument)
+        return self._signoff_helper(
+            object_id, 'get', template, uri_kwargs,
+            signoff_type=signoff_type, instrument=instrument
+        )
 
     @event_or_superevent
     def create_signoff(self, object_id, signoff_type, status, comment,
@@ -2605,8 +2606,10 @@ class GraceDb(GsiRest):
         else:
             raise NotImplementedError('Not yet implemented for events')
 
-        return self._signoff_helper(object_id, 'delete', template, uri_kwargs,
-                                    signoff_type, instrument)
+        return self._signoff_helper(
+            object_id, 'delete', template, uri_kwargs,
+            signoff_type=signoff_type, instrument=instrument
+        )
 
     def createTag(self, object_id, N, tag_name, displayName=None, *args,
                   **kwargs):