DM-35469: Make the storage class redefinition error message clearer

python/lsst/daf/butler/core/config.py

@@ -1190,6 +1190,8 @@ def __init__(self, other=None, validate=True, mergeDefaults=True, searchPaths=No
         # Now update this object with the external values so that the external
         # values always override the defaults
         self.update(externalConfig)
+        if not self.configFile:
+            self.configFile = externalConfig.configFile
 
         # If this configuration has child configurations of the same
         # config class, we need to expand those defaults as well.

python/lsst/daf/butler/core/storageClass.py

@@ -27,6 +27,7 @@
 
 import builtins
 import copy
+import itertools
 import logging
 from typing import Any, Collection, Dict, List, Mapping, Optional, Sequence, Set, Tuple, Type, Union
 
@@ -660,7 +661,7 @@ def addFromConfig(self, config: Union[StorageClassConfig, Config, str]) -> None:
         # components or parents before their classes are defined
         # we have a helper function that we can call recursively
         # to extract definitions from the configuration.
-        def processStorageClass(name: str, sconfig: StorageClassConfig) -> None:
+        def processStorageClass(name: str, sconfig: StorageClassConfig, msg: str = "") -> None:
             # Maybe we've already processed this through recursion
             if name not in sconfig:
                 return
@@ -683,7 +684,7 @@ def processStorageClass(name: str, sconfig: StorageClassConfig) -> None:
                 components = {}
                 for cname, ctype in info[compName].items():
                     if ctype not in self:
-                        processStorageClass(ctype, sconfig)
+                        processStorageClass(ctype, sconfig, msg)
                     components[cname] = self.getStorageClass(ctype)
 
                 # Fill in other items
@@ -694,15 +695,21 @@ def processStorageClass(name: str, sconfig: StorageClassConfig) -> None:
             if "inheritsFrom" in info:
                 baseName = info["inheritsFrom"]
                 if baseName not in self:
-                    processStorageClass(baseName, sconfig)
+                    processStorageClass(baseName, sconfig, msg)
                 baseClass = type(self.getStorageClass(baseName))
 
             newStorageClassType = self.makeNewStorageClass(name, baseClass, **storageClassKwargs)
             newStorageClass = newStorageClassType()
-            self.registerStorageClass(newStorageClass)
+            self.registerStorageClass(newStorageClass, msg=msg)
+
+        # In case there is a problem, construct a context message for any
+        # error reporting.
+        files = [str(f) for f in itertools.chain([sconfig.configFile], sconfig.filesRead) if f]
+        context = f"when adding definitions from {', '.join(files)}" if files else ""
+        log.debug("Adding definitions from config %s", ", ".join(files))
 
         for name in list(sconfig.keys()):
-            processStorageClass(name, sconfig)
+            processStorageClass(name, sconfig, context)
 
     @staticmethod
     def makeNewStorageClass(
@@ -778,7 +785,7 @@ def getStorageClass(self, storageClassName: str) -> StorageClass:
         """
         return self._storageClasses[storageClassName]
 
-    def registerStorageClass(self, storageClass: StorageClass) -> None:
+    def registerStorageClass(self, storageClass: StorageClass, msg: Optional[str] = None) -> None:
         """Store the `StorageClass` in the factory.
 
         Will be indexed by `StorageClass.name` and will return instances
@@ -788,19 +795,22 @@ def registerStorageClass(self, storageClass: StorageClass) -> None:
         ----------
         storageClass : `StorageClass`
             Type of the Python `StorageClass` to register.
+        msg : `str`, optional
+            Additional message string to be included in any error message.
 
         Raises
         ------
         ValueError
             If a storage class has already been registered with
-            storageClassName and the previous definition differs.
+            that storage class name and the previous definition differs.
         """
         if storageClass.name in self._storageClasses:
             existing = self.getStorageClass(storageClass.name)
             if existing != storageClass:
+                errmsg = f" {msg}" if msg else ""
                 raise ValueError(
-                    f"New definition for StorageClass {storageClass.name} ({storageClass}) "
-                    f"differs from current definition ({existing})"
+                    f"New definition for StorageClass {storageClass.name} ({storageClass!r}) "
+                    f"differs from current definition ({existing!r}){errmsg}"
                 )
         else:
             self._storageClasses[storageClass.name] = storageClass

tests/test_storageClass.py

@@ -167,8 +167,12 @@ def testRegistry(self):
         # Make sure we can't register a storage class with the same name
         # but different values
         newclass3 = StorageClass("Temporary2", pytype=dict)
-        with self.assertRaises(ValueError):
+        with self.assertRaises(ValueError) as cm:
             factory.registerStorageClass(newclass3)
+        self.assertIn("pytype='dict'", str(cm.exception))
+        with self.assertRaises(ValueError) as cm:
+            factory.registerStorageClass(newclass3, msg="error string")
+        self.assertIn("error string", str(cm.exception))
 
         factory._unregisterStorageClass(newclass3.name)
         self.assertNotIn(newclass3, factory)