fix: update migration code to account for missing fields #30
Conversation
satra
added
patch
Codecov Report
@@ Coverage Diff @@
## master #30 +/- ##
==========================================
+ Coverage 94.52% 94.74% +0.22%
==========================================
Files 11 11
Lines 858 876 +18
==========================================
+ Hits 811 830 +19
+ Misses 47 46 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@yarikoptic - a quick review of this would be much appreciated. regarding adding newlines. they were added to the schema from the schema precommit action when i pushed manually. i'm just adding that here as a default mechanism when we write out the schemas. |
dandischema/metadata.py
Outdated
| (vdir / "dandiset.json").write_text(models.Dandiset.schema_json(indent=2)) | ||
| (vdir / "asset.json").write_text(models.Asset.schema_json(indent=2)) | ||
| (vdir / "dandiset.json").write_text( | ||
| models.Dandiset.schema_json(indent=2).rstrip("\n") + "\n" |
There was a problem hiding this comment.
absent trailing new line was an issue?
we don't care about windows here, or python would magically convert line breaks to windows ones?
I would have probably came up with a helper ensure_trailing_space or alike to avoid duplication.
There was a problem hiding this comment.
or better, as I have suggested before, I think we should overload schema_json and centralize all our tuneups (indent and trailing line) in there
There was a problem hiding this comment.
python magically converts or rather both open/write_text does as far as i could tell.
There was a problem hiding this comment.
i'm still a little hesitant on overwriting schema_json as then we would need to ensure that everything we have is derived from a common BaseModel that just overrides schema_json. doable, but hesitant.
There was a problem hiding this comment.
isn't everything relevant here is a subclass of DandiBaseModel or otherwise we would have other issues (since DandiBaseModel does already to_dictrepr and unvalidated and we would fail if it didn't?). Anyways -- could be harmonized after this PR. But the question just keeps coming, so we better deal with it.... I guess next time ;)
Co-authored-by: Yaroslav Halchenko <[email protected]>
|
can i merge this now? |
|
Sure ;-) Let's see how release goes (no new release of schema but new release of library) |
I just ran through all the dandiset metadata and this code wasn't taking badly formed metadata into account.