DM-48074: Introduce keyCheck callback function for DictField and ConfigDictField #113
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
+ Coverage 85.87% 86.00% +0.13%
==========================================
Files 46 46
Lines 3604 3645 +41
==========================================
+ Hits 3095 3135 +40
- Misses 509 510 +1 ☔ View full report in Codecov by Sentry. |
enourbakhsh
force-pushed
the
tickets/DM-48074
branch
from
950dd47 to
ce0f96c
Compare
enourbakhsh
force-pushed
the
tickets/DM-48074
branch
from
48147ce to
d2f50e8
Compare
enourbakhsh
force-pushed
the
tickets/DM-48074
branch
from
cfdd38c to
7b5464c
Compare
| # 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) |
There was a problem hiding this comment.
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.
| @@ -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.
These errors could actually be consolidated into one, e.g.:
check_errors = []
for name, check in (("dictCheck", dictCheck), ("keyCheck", keyCheck), ("itemCheck", itemCheck)):
if check is not None and not hasattr(check, "__call__"):
check_errors.append(name)
if check_errors:
raise ValueError(f"{', '.join(check_errors)} must be callable")
There was a problem hiding this comment.
Good idea, I just wanted to be minimally invasive in my approach but this sounds like an improvement.
python/lsst/pex/config/dictField.py
Outdated
| @@ -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.
Same comment about consolidating errors here. Also it seems like a lot of code could have been shared between configDictField and regular dictField, but that's another issue for another day.
| 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.
I assume you've checked that these formats and the ones above are identical (it is not totally obvious to me).
python/lsst/pex/config/history.py
Outdated
|
|
||
| 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.
Any reason why the \n can't go inside the f-string?
There was a problem hiding this comment.
Nice catch! Moved it inside the f-string.
| class BadKeyCheck(pexConfig.Config): | ||
| d = pexConfig.DictField("...", keytype=int, itemtype=int, keyCheck=4) | ||
|
|
||
| except Exception: |
There was a problem hiding this comment.
Not ValueError, or a with self.assertRaises(ValueError, ... like below?
There was a problem hiding this comment.
I agree, and that does seem more suitable to me as well. However, there are four more try...except blocks in this test case, both above and below, and I didn’t want to break consistency. I think we should modernize all these old tests in a new ticket.
enourbakhsh
force-pushed
the
tickets/DM-48074
branch
from
ec07028 to
d47cbfd
Compare
Checklist
doc/changes