make schemaKey required and improve validation and migration functions #77
Conversation
Codecov Report
@@ Coverage Diff @@
## master #77 +/- ##
==========================================
+ Coverage 89.19% 89.44% +0.25%
==========================================
Files 13 14 +1
Lines 1342 1355 +13
==========================================
+ Hits 1197 1212 +15
+ Misses 145 143 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Could you please elaborate on what you mean by "handle" in this context? |
|
this PR (which has changed since the original attempt) now:
so handle means to always track |
satra
changed the title
|
@yarikoptic - i think this is good for a review/merge. i would still like to get seb and dorota's PRs in before release. |
dandischema/metadata.py
Outdated
| continue | ||
| error_list.append([error.message, tuple(error.absolute_path)]) | ||
| if error_list: | ||
| raise jsonschema.ValidationError(str(error_list)) |
There was a problem hiding this comment.
this would collapse all errors into one string of a list, making it impossible to print it nicely or "analyze" above.
I guess should be ok for now, but I think we might need to come up with some consistent mechanism between dandischema and dandi-cli to "encapsulate" multiple errors as a singular exception which would
- contain
.errorslist of all individual errors/exceptions (not sure if wrapped into some adapter here to provide consistent interface across errors) - have nice
__str__and/or__repr__
There was a problem hiding this comment.
it would be nice to have this. let me look into if python itself has a way of doing this.
dandischema/metadata.py
Outdated
| @@ -168,7 +195,7 @@ def validate(obj, schema_version=None, schema_key=None, missing_ok=False): | |||
| reraise = True | |||
| messages.append(el["msg"]) | |||
| if reraise: | |||
| ValueError(messages) | |||
| raise ValueError(messages) | |||
There was a problem hiding this comment.
and that would be another location to use the aforementioned encapsulation of errors.
Co-authored-by: Yaroslav Halchenko <[email protected]>
|
@yarikoptic - any further thoughts on this? |
dandischema/metadata.py
Outdated
| continue | ||
| error_list.append([error]) | ||
| if error_list: | ||
| raise ValueError(error_list) |
There was a problem hiding this comment.
Echoing prior discussion, Let's at least define our own class ValidationError(ValueError) may be?
There was a problem hiding this comment.
do you want to do it now? or later?
There was a problem hiding this comment.
If no objections, will push that change in 5 minutes. Better not to delay ;-)
There was a problem hiding this comment.
Later we would need to reactor anyways though
There was a problem hiding this comment.
any other changes you plan, or should i merge?
Otherwise we were reraising original (non-documented) pydantic exception in case of not missing_ok PS also added forgotten in previous commit exceptions.py
yarikoptic
force-pushed
the
enh/migrate
branch
from
1b73409 to
92d7d59
Compare
@yarikoptic - this is an initial attempt to handle
schemaKey, but this requires a weird thing from a python perspective.in pydantic, a class is initialized with
kwargs. this PR addsschemaKeytokwargsto get the validator to trigger a mismatch. this works for passing data from an existing document. however, this also means that any initialization of the class will also require the appropriateschemaKey. see thetest_missing_schemakeyin the PR. this is quite weird.the alternative is a function that checks every object (i.e. dictionary) in a json record for the presence of
schemaKey, but then this function would need to be called before initializing the class, which also seems weird.things addressed