code review fixes

python/lsst/daf/persistence/policy.py

@@ -75,15 +75,15 @@ def __init__(self, other=None):
         other (Pex Policy) Initializes this Policy with the values in the passed-in Pex Policy.
         other (Policy) Copies the other Policy's values into this one.
         other (dict) Copies the values from the dict into this Policy.
-    
+
         """
         collections.UserDict.__init__(self)
-        
+
         if other is None:
             return
 
         try:
-            other.keys() # is it a dict?
+            other.keys()  # is it a dict?
             self.update(other)
             return
         except AttributeError:
@@ -130,7 +130,6 @@ def __initFromFile(self, path):
         else:
             raise RuntimeError("Unhandled policy file type:%s" % path)
 
-
     def __initFromPexPolicy(self, pexPolicy):
         """Load values from a pex policy.
 
@@ -211,21 +210,27 @@ def __contains__(self, key):
         return keys[-1] in d
 
     @staticmethod
-    def defaultPolicyFile(productName, filePath, repos=None):
-        """Get the path to a default policy file
+    def defaultPolicyFile(productName, fileName, relativePath=None):
+        """Get the path to a default policy file.
+
+        Determines a directory for the product specified by productName. Then Concatenates
+        productDir/relativePath/fileName (or productDir/fileName if relativePath is None) to find the path
+        to the default Policy file
 
         @param productName (string) The name of the product that the default policy is installed as part of
-        @param filepath (string) The relative pathname to the policy file.  
-        @param repos (string) the subdirectory with the product's install directory where policy files are 
-                              stored. If None (default), the filepath argument is relative to the installation
-                              directory.
+        @param fileName (string) The name of the policy file. Can also include a path to the file relative to
+                                 the directory where the product is installed.
+        @param relativePath (string) The relative path from the directior where the product is installed to
+                                     the location where the file (or the path to the file) is found. If None
+                                     (default), the fileName argument is relative to the installation
+                                     directory.
         """
-        basePath = os.getenv(productName.upper() + "_DIR")
+        basePath = lsst.utils.getPackageDir(productName)
         if not basePath:
             raise RuntimeError("No product installed for productName: %s" % basePath)
-        if repos is not None:
-            basePath = os.path.join(basePath, repos)
-        fullFilePath = os.path.join(basePath, filePath)
+        if relativePath is not None:
+            basePath = os.path.join(basePath, relativePath)
+        fullFilePath = os.path.join(basePath, fileName)
         return fullFilePath
 
     def update(self, other):
@@ -415,7 +420,7 @@ def dump(self, output):
         :return:
         """
         # First a set of known keys is handled and written to the stream in a specific order for readability.
-        # After the expected/ordered keys are weritten to the stream the remainder of the keys are written to 
+        # After the expected/ordered keys are weritten to the stream the remainder of the keys are written to
         # the stream.
         data = copy.copy(self.data)
         keys = ['defects', 'needCalibRegistry', 'levels', 'defaultLevel', 'defaultSubLevels',

tests/testPolicy.py

@@ -34,7 +34,7 @@ class PolicyTestCase(unittest.TestCase):
 
     def setUp(self):
         self.policy = Policy(
-            {'body': {'job': {'position': 'Developer', 'company': 'Microsoft'}, 'name': 'John'}, 
+            {'body': {'job': {'position': 'Developer', 'company': 'Microsoft'}, 'name': 'John'},
              'error': False})
 
     def testBasic(self):
@@ -122,12 +122,14 @@ def testDumpLoad(self):
         os.remove('testDumpFile.yaml')
 
     def testNonExistantPolicyAtPath(self):
-        self.assertRaises(IOError, Policy, "c:/policy.yaml")
-        self.assertRaises(IOError, Policy, "c:/policy.paf")
+        # Test that loading paf & yaml files that do not exist raises an IOError.
+        self.assertRaises(IOError, Policy, "c:/nonExistantPath/policy.yaml")
+        self.assertRaises(IOError, Policy, "c:/nonExistantPath/policy.paf")
 
     def testInvalidPolicyFileType(self):
-        # we only support files with '.paf' or '.yaml' extension
-        self.assertRaises(RuntimeError, Policy, "c:/policy") 
+        # We only support files with '.paf' or '.yaml' extension; check that trying to load a file with a
+        # different extension (in this case '.txt') raises a RuntimeError.
+        self.assertRaises(RuntimeError, Policy, "c:/policy.txt")
 
 
 class TestMemory(lsst.utils.tests.MemoryTestCase):