-
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
Improve migrate()
in metadata.py
#279
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #279 +/- ##
==========================================
- Coverage 97.54% 92.45% -5.10%
==========================================
Files 16 16
Lines 1753 1775 +22
==========================================
- Hits 1710 1641 -69
- Misses 43 134 +91
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
d5aef64
to
3c1c3cd
Compare
note that tests are failing ATM, e.g.
|
@yarikoptic Tests are updated. I think this PR is ready to be merged once you are OK with it. |
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.
make a bit more consistent
dandischema/metadata.py
Outdated
obj["schemaKey"] = "Dandiset" | ||
obj["schemaVersion"] = to_version | ||
return obj | ||
if "schemaKey" not in instance_migrated["assetsSummary"]: |
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.
if "schemaKey" not in instance_migrated["assetsSummary"]: | |
if "schemaKey" not in instance_migrated.get("assetsSummary", {}): |
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 replaced the line with the following instead.
if (
"assetsSummary" in obj_migrated
and "schemaKey" not in obj_migrated["assetsSummary"]
):
3a4ebdc
to
d0baff5
Compare
This commit improves `migrate()` by removing bugs, making it more readable, adding argument validations, etc. However, it doesn't change the migration of metadata instances this function currently intend to implement, per my best guess. The following are descriptions of the more prominent improvements: 1. Correct the type annotation of `to_version` (Bug fix) 2. Validate that the provided metadata instance and the target version specifies valid DANDI schema versions 3. Ensure the optional validation of the provided instance against its specified schema is done before any migration operation, to achieve greater consistency. 4. Return the instance unchanged when DANDI schema version of the given instance is equal to the target version instead (Bug fix). 5. Consistently use the tuple representations of DANDI schema versions to route the execution flow of migration operations to ensure all cases of the relation between the schema version of the instance and target version are covered. 6. Condition the migration to "0.6.0" and beyond on the target version being at least "0.6.0". (Bug fix) 7. Copy input instance for migration only immediately before migration operations 7. Rename local variables to improve readability 8. Handle the situation in which the input metadata instance not having the "assetsSummary" field Co-authored-by: Yaroslav Halchenko <[email protected]> Co-authored-by: John T. Wodder II <[email protected]>
d0baff5
to
89e2cb7
Compare
@yarikoptic All the issues have been handled. Please do a final check and merge. |
This PR improves
migrate()
by removing bugs, making it more readable, adding argument validations, etc. Please see commit message for more details of the improvements.