Merge pull request apel#366 from tofu-rocketry/code-quality

Code quality

apel/db/loader/loader.py

@@ -93,10 +93,9 @@ def startup(self):
             log.warning("Check that the dbloader is not running, then remove the file.")
             raise LoaderException("The dbloader cannot start while pidfile exists.")
         try:
-            f = open(self._pidfile, "w")
-            f.write(str(os.getpid()))
-            f.write("\n")
-            f.close()
+            with open(self._pidfile, "w") as f:
+                f.write(str(os.getpid()))
+                f.write("\n")
         except IOError as e:
             log.warning("Failed to create pidfile %s: %s", self._pidfile, e)
 

apel/db/unloader.py

@@ -263,7 +263,6 @@ def _write_xml(self, records):
 
         self._msgq.add(buf.getvalue())
         buf.close()
-        del buf
 
     def _write_apel(self, records):
         '''
@@ -279,7 +278,7 @@ def _write_apel(self, records):
 
         self._msgq.add(buf.getvalue())
         buf.close()
-        del buf
+
 
 def get_start_of_previous_month(dt):
     '''

bin/parser.py

@@ -201,17 +201,14 @@ def scan_dir(parser, dirpath, reparse, expr, apel_db, processed):
                         # format we will get IOError, empty files can
                         # give EOFError as well.
                         for method in (bz2.BZ2File, gzip.open, open):
-                            try:  # this is for Python < 2.5
-                                try:
-                                    fp = method(abs_file, 'rb')
+                            try:
+                                with method(abs_file, 'rb') as fp:
                                     parsed, total = parse_file(parser, apel_db,
                                                                fp, reparse)
-                                    break
-                                except (IOError, EOFError):
-                                    if method == open:
-                                        raise
-                            finally:
-                                fp.close()
+                                break
+                            except (IOError, EOFError):
+                                if method == open:
+                                    raise
                     except IOError as e:
                         log.error('Cannot parse file %s: %s', item, e)
                     except ApelDbException as e:

bin/retrieve_dns.py

@@ -201,11 +201,10 @@ def dns_from_file(path):
     assume that they're DNs, but we'll check later.
     '''
 
-    dn_file = open(path)
-    dns = dn_file.readlines()
+    with open(path) as dn_file:
+        dns = dn_file.readlines()
     # get rid of any whitespace in the list of strings
     dns = [dn.strip() for dn in dns]
-    dn_file.close()
     return dns
 
 
@@ -293,7 +292,7 @@ def runprocess(config_file, log_config_file):
                 log.warning('Is the URL correct?')
                 fetch_failed = True
     except AttributeError:
-        # gocdb_url == None
+        # gocdb_url is None
         log.info("No GOCDB URL specified - won't fetch URLs.")
     except IOError as e:
         log.info("Failed to retrieve XML - is the URL correct?")

bin/summariser.py

@@ -100,10 +100,9 @@ def runprocess(db_config_file, config_file, log_config_file):
         print("Error initialising summariser: %s" % err)
         sys.exit(1)
     try:
-        f = open(pidfile, "w")
-        f.write(str(os.getpid()))
-        f.write("\n")
-        f.close()
+        with open(pidfile, "w") as f:
+            f.write(str(os.getpid()))
+            f.write("\n")
     except IOError as e:
         log.warning("Failed to create pidfile %s: %s", pidfile, e)
         # If we fail to create a pidfile, don't start the summariser

scripts/migrate_apel.py

@@ -146,13 +146,13 @@ def copy_records(db1, db2, cutoff):
 
         role, group, vo = parse_fqan(fqan)
 
-        if global_user_name == None:
+        if global_user_name is None:
             global_user_name = 'None'
-        if role == None:
+        if role is None:
             role = 'None'
-        if group == None:
+        if group is None:
             group = 'None'
-        if vo == None:
+        if vo is None:
             vo = 'None'
 
         start_time = parse_timestamp(start_time)

test/test_aur_parser.py

@@ -97,7 +97,7 @@ def test_aur_parser(self):
             for field in ('Site', 'Month', 'Year',
                           'WallDuration', 'CpuDuration', 'NumberOfJobs'):
                 # Also need: 'NormalisedWallDuration', 'NormalisedCpuDuration'
-                self.assertTrue(field in cont, "Field '%s' not found" % field)
+                self.assertIn(field, cont, "Field '%s' not found" % field)
 
             for key in cases[aur]:
                 if isinstance(cont[key], datetime):

test/test_blah.py

@@ -144,7 +144,7 @@ def test_multiple_fqans(self):
 
             for key in list(cases[line].keys()):
                 # Check all fields are present
-                self.assertTrue(key in cont, "Key '%s' not in record." % key)
+                self.assertIn(key, cont, "Key '%s' not in record." % key)
                 # Check values are correct
                 self.assertEqual(cont[key], cases[line][key],
                                  "'%s' != '%s' for key '%s'" %

test/test_car_parser.py

@@ -170,13 +170,13 @@ def test_car_parser(self):
             cont = record._record_content
 
             #  Mandatory fields - need checking.
-            self.assertTrue('Site' in cont)
-            self.assertTrue('Queue' in cont)
-            self.assertTrue('LocalJobId' in cont)
-            self.assertTrue('WallDuration' in cont)
-            self.assertTrue('CpuDuration' in cont)
-            self.assertTrue('StartTime' in cont)
-            self.assertTrue('EndTime' in cont)
+            self.assertIn('Site', cont)
+            self.assertIn('Queue', cont)
+            self.assertIn('LocalJobId', cont)
+            self.assertIn('WallDuration', cont)
+            self.assertIn('CpuDuration', cont)
+            self.assertIn('StartTime', cont)
+            self.assertIn('EndTime', cont)
 
             for key in list(cases[car].keys()):
                 if isinstance(cont[key], datetime.datetime):

test/test_hashing.py

@@ -16,9 +16,8 @@ def test_calculate_hash(self):
         data_hash = '3d1eb00cc63828b36882f076f35c8cdd'
 
         tmpname = tempfile.mktemp('hashtest')
-        fp = open(tmpname, 'wb')
-        fp.write(data)
-        fp.close()
+        with open(tmpname, 'wb') as fp:
+            fp.write(data)
 
         self.assertEqual(data_hash, calculate_hash(tmpname))
 

test/test_htcondor_parser.py

@@ -66,7 +66,7 @@ def test_parse_line_no_cputmult(self):
             self.assertEqual(cont['Infrastructure'], 'APEL-CREAM-HTCONDOR')
 
             for key in list(cases[line].keys()):
-                self.assertTrue(key in cont, "Key '%s' not in record." % key)
+                self.assertIn(key, cont, "Key '%s' not in record." % key)
 
             for key in list(cases[line].keys()):
                 self.assertEqual(cont[key], cases[line][key],
@@ -125,7 +125,7 @@ def test_parse_line_with_cputmult(self):
             self.assertEqual(cont['Infrastructure'], 'APEL-CREAM-HTCONDOR')
 
             for key in list(cases[line].keys()):
-                self.assertTrue(key in cont, "Key '%s' not in record." % key)
+                self.assertIn(key, cont, "Key '%s' not in record." % key)
 
             for key in list(cases[line].keys()):
                 self.assertEqual(cont[key], cases[line][key],

test/test_logging.py

@@ -50,9 +50,8 @@ def test_stdout_logging(self):
 
         # Only check bit after timestamp as that doesn't change.
         self.assertEqual(output[23:], " - test_logging - INFO - out\n")
-        f = open(self.path)
-        self.assertEqual(f.readline()[23:], " - test_logging - INFO - out\n")
-        f.close()
+        with open(self.path) as f:
+            self.assertEqual(f.readline()[23:], " - test_logging - INFO - out\n")
 
     def test_boring_logging(self):
         """Check that logging without handlers at least runs without errors."""

test/test_lsf.py

@@ -107,7 +107,7 @@ def test_parse(self):
             self.assertEqual(cont['Infrastructure'], 'APEL-CREAM-LSF')
 
             for field in list(cases[line].keys()):
-                self.assertTrue(field in cont, "Field '%s' not in record: %s" % (field, cont))
+                self.assertIn(field, cont, "Field '%s' not in record: %s" % (field, cont))
 
             for key in list(cases[line].keys()):
                 self.assertEqual(cont[key], cases[line][key], "%s != %s for key %s" % (cont[key], cases[line][key], key))

test/test_pbs.py

@@ -114,7 +114,7 @@ def test_parse(self):
 
             for key in list(cases[line].keys()):
                 # Check all fields are present
-                self.assertTrue(key in cont, "Key '%s' not in record." % key)
+                self.assertIn(key, cont, "Key '%s' not in record." % key)
                 # Check values are correct
                 self.assertEqual(cont[key], cases[line][key], "%s != %s for key %s" % (cont[key], cases[line][key], key))
 

test/test_record_factory.py

@@ -32,7 +32,7 @@ def test_create_car(self):
             '<urf:UsageRecord xmlns:urf="http://eu-emi.eu/namespaces/2012/11/co'
             'mputerecord"></urf:UsageRecord>'
         )
-        self.assertTrue(isinstance(record_list[0], JobRecord))
+        self.assertIsInstance(record_list[0], JobRecord)
 
     def test_create_aur(self):
         """Check that trying to create an AUR record fails."""
@@ -48,7 +48,7 @@ def test_create_star(self):
             '02/storagerecord"><sr:RecordIdentity sr:createTime="2016-06-09T02:'
             '42:15Z" sr:recordId="c698"/></sr:StorageUsageRecord>'
         )
-        self.assertTrue(isinstance(record_list[0], StorageRecord))
+        self.assertIsInstance(record_list[0], StorageRecord)
 
     def test_create_bad_xml(self):
         """Check that trying to create a record from non-record XML fails."""
@@ -63,36 +63,36 @@ def test_create_bad_apel(self):
     def test_create_jrs(self):
         """Check that creating job records returns JobRecords."""
         records = self._rf.create_records(self._jr_text)
-        self.assertTrue(len(records), 2)
+        self.assertEqual(len(records), 2)
         for record in records:
-            self.assertTrue(isinstance(record, JobRecord))
+            self.assertIsInstance(record, JobRecord)
 
     def test_create_srs(self):
         """Check that creating summary records returns SummaryRecords."""
         records = self._rf.create_records(self._sr_text)
-        self.assertTrue(len(records), 2)
+        self.assertEqual(len(records), 2)
         for record in records:
-            self.assertTrue(isinstance(record, SummaryRecord))
+            self.assertIsInstance(record, SummaryRecord)
 
     def test_create_normalised_summary(self):
         """Check that creating a normalised summary returns the right record."""
         records = self._rf.create_records(self._nsr_text)
-        self.assertTrue(isinstance(records[0], NormalisedSummaryRecord))
+        self.assertIsInstance(records[0], NormalisedSummaryRecord)
 
     def test_create_sync(self):
         """Check that creating a sync record returns a SyncRecord."""
         records = self._rf.create_records(self._sync_text)
-        self.assertTrue(isinstance(records[0], SyncRecord))
+        self.assertIsInstance(records[0], SyncRecord)
 
     def test_create_cloud(self):
         """Check that creating a cloud record returns a CloudRecord."""
         records = self._rf.create_records(self._cr_text)
-        self.assertTrue(isinstance(records[0], CloudRecord))
+        self.assertIsInstance(records[0], CloudRecord)
 
     def test_create_cloud_summary(self):
         """Check that creating a cloud summary returns a CloudSummaryRecord."""
         records = self._rf.create_records(self._csr_text)
-        self.assertTrue(isinstance(records[0], CloudSummaryRecord))
+        self.assertIsInstance(records[0], CloudSummaryRecord)
 
     def _get_msg_text(self):
     # Below, I've just got some test data hard-coded.

test/test_slurm.py

@@ -115,7 +115,7 @@ def test_parse_line(self):
                 del cases[line]['Queue']
 
             for key in list(cases[line].keys()):
-                self.assertTrue(key in cont, "Key '%s' not in record." % key)
+                self.assertIn(key, cont, "Key '%s' not in record." % key)
 
             for key in list(cases[line].keys()):
                 self.assertEqual(cont[key], cases[line][key], "%s != %s for key %s." % (cont[key], cases[line][key], key))

test/test_star_parser.py

@@ -110,7 +110,7 @@ def test_star_parser(self):
             #  Mandatory fields
             for field in ("RecordId", "CreateTime", "StorageSystem",
                           "StartTime", "EndTime", "ResourceCapacityUsed"):
-                self.assertTrue(field in cont, "Field '%s' not found" % field)
+                self.assertIn(field, cont, "Field '%s' not found" % field)
 
             for key in list(cases[star].keys()):
                 if isinstance(cont[key], datetime):