Skip to content

Commit

Permalink
Add extra context to storage class registration error
Browse files Browse the repository at this point in the history
Try to include the files that are being added.
  • Loading branch information
timj committed Jul 6, 2022
1 parent 117fec9 commit 3b87561
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 8 deletions.
26 changes: 18 additions & 8 deletions python/lsst/daf/butler/core/storageClass.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand All @@ -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!r}) "
f"differs from current definition ({existing!r})"
f"differs from current definition ({existing!r}){errmsg}"
)
else:
self._storageClasses[storageClass.name] = storageClass
Expand Down
3 changes: 3 additions & 0 deletions tests/test_storageClass.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ def testRegistry(self):
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)
Expand Down

0 comments on commit 3b87561

Please sign in to comment.