-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-48074: Introduce keyCheck callback function for DictField and ConfigDictField #113
Changes from 9 commits
087bee1
8858de3
349df56
ce0f96c
252e676
d2f50e8
6bf27ec
bdc8aa5
7b5464c
d47cbfd
a3a6a69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Introduced the `keyCheck` callback to `DictField` and `ConfigDictField`, allowing custom key validation during assignment. Added unit tests to ensure functionality. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,11 @@ def __setitem__(self, k, x, at=None, label="setitem", setHistory=True): | |
) | ||
raise FieldValidationError(self._field, self._config, msg) | ||
|
||
# validate key using keycheck | ||
if self._field.keyCheck is not None and not self._field.keyCheck(k): | ||
msg = f"Key {k!r} is not a valid key" | ||
raise FieldValidationError(self._field, self._config, msg) | ||
|
||
if at is None: | ||
at = getCallStack() | ||
name = _joinNamePath(self._config._name, self._field.name, k) | ||
|
@@ -127,6 +132,8 @@ class ConfigDictField(DictField): | |
Default is `True`. | ||
dictCheck : `~collections.abc.Callable` or `None`, optional | ||
Callable to check a dict. | ||
keyCheck : `~collections.abc.Callable` or `None`, optional | ||
Callable to check a key. | ||
itemCheck : `~collections.abc.Callable` or `None`, optional | ||
Callable to check an item. | ||
deprecated : None or `str`, optional | ||
|
@@ -140,7 +147,8 @@ class ConfigDictField(DictField): | |
|
||
- ``keytype`` or ``itemtype`` arguments are not supported types | ||
(members of `ConfigDictField.supportedTypes`. | ||
- ``dictCheck`` or ``itemCheck`` is not a callable function. | ||
- ``dictCheck``, ``keyCheck`` or ``itemCheck`` is not a callable | ||
function. | ||
|
||
See Also | ||
-------- | ||
|
@@ -172,6 +180,7 @@ def __init__( | |
default=None, | ||
optional=False, | ||
dictCheck=None, | ||
keyCheck=None, | ||
itemCheck=None, | ||
deprecated=None, | ||
): | ||
|
@@ -191,12 +200,15 @@ def __init__( | |
raise ValueError(f"'itemtype' {_typeStr(itemtype)} is not a supported type") | ||
if dictCheck is not None and not hasattr(dictCheck, "__call__"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These errors could actually be consolidated into one, e.g.:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, I just wanted to be minimally invasive in my approach but this sounds like an improvement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
raise ValueError("'dictCheck' must be callable") | ||
if keyCheck is not None and not hasattr(keyCheck, "__call__"): | ||
raise ValueError("'keyCheck' must be callable") | ||
if itemCheck is not None and not hasattr(itemCheck, "__call__"): | ||
raise ValueError("'itemCheck' must be callable") | ||
|
||
self.keytype = keytype | ||
self.itemtype = itemtype | ||
self.dictCheck = dictCheck | ||
self.keyCheck = keyCheck | ||
self.itemCheck = itemCheck | ||
|
||
def rename(self, instance): | ||
|
@@ -207,6 +219,23 @@ def rename(self, instance): | |
configDict[k]._rename(fullname) | ||
|
||
def validate(self, instance): | ||
"""Validate the field. | ||
|
||
Parameters | ||
---------- | ||
instance : `lsst.pex.config.Config` | ||
The config instance that contains this field. | ||
|
||
Raises | ||
------ | ||
lsst.pex.config.FieldValidationError | ||
Raised if validation fails for this field. | ||
|
||
Notes | ||
----- | ||
Individual key checks (``keyCheck``) are applied when each key is added | ||
and are not re-checked by this method. | ||
""" | ||
value = self.__get__(instance) | ||
if value is not None: | ||
for k in value: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -139,6 +139,11 @@ def __setitem__( | |
) | ||
raise FieldValidationError(self._field, self._config, msg) | ||
|
||
# validate key using keycheck | ||
if self._field.keyCheck is not None and not self._field.keyCheck(k): | ||
msg = f"Key {k!r} is not a valid key" | ||
raise FieldValidationError(self._field, self._config, msg) | ||
|
||
# validate item using itemcheck | ||
if self._field.itemCheck is not None and not self._field.itemCheck(x): | ||
msg = f"Item at key {k!r} is not a valid value: {x}" | ||
|
@@ -214,6 +219,8 @@ class DictField(Field[Dict[KeyTypeVar, ItemTypeVar]], Generic[KeyTypeVar, ItemTy | |
If `True`, the field doesn't need to have a set value. | ||
dictCheck : callable | ||
A function that validates the dictionary as a whole. | ||
keyCheck : callable | ||
A function that validates individual mapping keys. | ||
itemCheck : callable | ||
A function that validates individual mapping values. | ||
deprecated : None or `str`, optional | ||
|
@@ -289,6 +296,7 @@ def __init__( | |
default=None, | ||
optional=False, | ||
dictCheck=None, | ||
keyCheck=None, | ||
itemCheck=None, | ||
deprecated=None, | ||
): | ||
|
@@ -312,12 +320,15 @@ def __init__( | |
raise ValueError(f"'itemtype' {_typeStr(itemtype)} is not a supported type") | ||
if dictCheck is not None and not hasattr(dictCheck, "__call__"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment about consolidating errors here. Also it seems like a lot of code could have been shared between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
raise ValueError("'dictCheck' must be callable") | ||
if keyCheck is not None and not hasattr(keyCheck, "__call__"): | ||
raise ValueError("'keyCheck' must be callable") | ||
if itemCheck is not None and not hasattr(itemCheck, "__call__"): | ||
raise ValueError("'itemCheck' must be callable") | ||
|
||
self.keytype = keytype | ||
self.itemtype = itemtype | ||
self.dictCheck = dictCheck | ||
self.keyCheck = keyCheck | ||
self.itemCheck = itemCheck | ||
|
||
def validate(self, instance): | ||
|
@@ -328,23 +339,22 @@ def validate(self, instance): | |
instance : `lsst.pex.config.Config` | ||
The configuration that contains this field. | ||
|
||
Returns | ||
------- | ||
isValid : `bool` | ||
`True` is returned if the field passes validation criteria (see | ||
*Notes*). Otherwise `False`. | ||
Raises | ||
------ | ||
lsst.pex.config.FieldValidationError | ||
Raised if validation fails for this field (see *Notes*). | ||
|
||
Notes | ||
----- | ||
This method validates values according to the following criteria: | ||
|
||
- A non-optional field is not `None`. | ||
- If a value is not `None`, is must pass the `ConfigField.dictCheck` | ||
user callback functon. | ||
- If a value is not `None`, it must pass the `ConfigField.dictCheck` | ||
user callback function. | ||
|
||
Individual item checks by the `ConfigField.itemCheck` user callback | ||
function are done immediately when the value is set on a key. Those | ||
checks are not repeated by this method. | ||
Individual key and item checks by the ``keyCheck`` and ``itemCheck`` | ||
user callback functions are done immediately when the value is set on a | ||
key. Those checks are not repeated by this method. | ||
""" | ||
Field.validate(self, instance) | ||
value = self.__get__(instance) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -214,7 +214,7 @@ | |
if writeSourceLine: | ||
line.append( | ||
[ | ||
"%s" % ("%s:%d" % (frame.filename, frame.lineno)), | ||
f"{frame.filename}:{frame.lineno}", | ||
"FILE", | ||
] | ||
) | ||
|
@@ -252,14 +252,14 @@ | |
fullname = f"{config._name}.{name}" if config._name is not None else name | ||
msg.append(_colorize(re.sub(r"^root\.", "", fullname), "NAME")) | ||
for value, output in outputs: | ||
line = prefix + _colorize("%-*s" % (valueLength, value), "VALUE") + " " | ||
line = prefix + _colorize(f"{value:<{valueLength}}", "VALUE") + " " | ||
for i, vt in enumerate(output): | ||
if writeSourceLine: | ||
vt[0][0] = "%-*s" % (sourceLength, vt[0][0]) | ||
vt[0][0] = f"{vt[0][0]:<{sourceLength}}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you've checked that these formats and the ones above are identical (it is not totally obvious to me). |
||
|
||
output[i] = " ".join([_colorize(v, t) for v, t in vt]) | ||
|
||
line += ("\n%*s" % (valueLength + 1, "")).join(output) | ||
line += ("\n" + f"{'':>{valueLength + 1}}").join(output) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch! Moved it inside the f-string. |
||
msg.append(line) | ||
|
||
return "\n".join(msg) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,8 @@ | |
d2 = pexConfig.DictField("d2", keytype=str, itemtype=str, default=None) | ||
d3 = pexConfig.DictField("d3", keytype=float, itemtype=float, optional=True, itemCheck=lambda x: x > 0) | ||
d4 = pexConfig.DictField("d4", keytype=str, itemtype=None, default={}) | ||
d5 = pexConfig.DictField[str, float]("d5", default={}, keyCheck=lambda k: k not in ["k1", "k2"]) | ||
d6 = pexConfig.DictField[int, str]("d6", default={-2: "v1", 4: "v2"}, keyCheck=lambda k: k % 2 == 0) | ||
|
||
|
||
class DictFieldTest(unittest.TestCase): | ||
|
@@ -64,6 +66,16 @@ | |
else: | ||
raise SyntaxError("Unsupported itemtype DictFields should not be allowed") | ||
|
||
try: | ||
|
||
class BadKeyCheck(pexConfig.Config): | ||
d = pexConfig.DictField("...", keytype=int, itemtype=int, keyCheck=4) | ||
|
||
except Exception: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, and that does seem more suitable to me as well. However, there are four more |
||
pass | ||
else: | ||
raise SyntaxError("Non-callable keyCheck DictFields should not be allowed") | ||
|
||
try: | ||
|
||
class BadItemCheck(pexConfig.Config): | ||
|
@@ -139,6 +151,23 @@ | |
c.d2 = {"a": "b"} | ||
c.validate() | ||
|
||
def testKeyCheckValidation(self): | ||
c = Config1() | ||
c.d5 = {"k3": -1, "k4": 0.25} | ||
c.d6 = {6: "v3"} | ||
|
||
with self.assertRaises( | ||
pexConfig.FieldValidationError, | ||
msg="Key check must reject dictionary assignment with invalid keys", | ||
): | ||
c.d5 = {"k1": 1.5, "k2": 2.0} | ||
|
||
with self.assertRaises( | ||
pexConfig.FieldValidationError, | ||
msg="Key check must reject invalid key addition", | ||
): | ||
c.d6[3] = "v4" | ||
|
||
def testInPlaceModification(self): | ||
c = Config1() | ||
self.assertRaises(pexConfig.FieldValidationError, c.d1.__setitem__, 2, 0) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of wish this method would collect all errors before throwing in case there are multiple, but I suppose that's not how it was designed. Oh well.