Skip to content
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

enh: allow checking to see data are matched but allow for missing fields #48

Merged
merged 5 commits into from
Jun 30, 2021

Conversation

satra
Copy link
Member

@satra satra commented Jun 25, 2021

adding a mechanism to allow checking that a metadata blob for dandiset or asset has proper values given the schema, but may have missing fields. these metadata objects should be ok for posting but not for publishing.

@dchiquito and @jwodder - would this be useful?

@codecov
Copy link

codecov bot commented Jun 25, 2021

Codecov Report

Merging #48 (bc9cced) into master (a22172f) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage   95.79%   95.87%   +0.07%     
==========================================
  Files          11       11              
  Lines        1023     1042      +19     
==========================================
+ Hits          980      999      +19     
  Misses         43       43              
Flag Coverage Δ
unittests 95.87% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dandischema/metadata.py 96.79% <100.00%> (+0.23%) ⬆️
dandischema/tests/test_metadata.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a22172f...bc9cced. Read the comment docs.

@jwodder
Copy link
Member

jwodder commented Jun 25, 2021

@satra I think it would be more useful to give Dandiset a base class akin to BareAsset that lacks the fields that the server sets.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If decided to proceed this way (I think @jwodder 's comment on a dedicated base class might be a more "kosher"), left a comment/suggestiong on needed adjustment to docstring

@satra
Copy link
Member Author

satra commented Jun 28, 2021

what this checks is that fields are valid when provided, not if they are required. in fact this explicitly does not care about required. for dandiset this is important at the API level so that metadata can be updated through the UI, and that a user could POST dandiset meta through the API.

i don't think a base class buys us anything in this use case. further fields required can change across schema versions, and hence any base class will also have to support this. a validation function can take that into account a lot more easily than attributes in a class.

Co-authored-by: Yaroslav Halchenko <[email protected]>
@yarikoptic
Copy link
Member

what this checks is that fields are valid when provided, not if they are required

oh, so it sounds like missing_ok: bool =False flag !? for_post was really not descriptive to me so I have missed the intent of the proposed change even though it was described ;)

would need some rudimentary test.

longer term: adding such a flag we are getting into the territory of flags/settings on what to ignore, and might end up breeding other boolean flags later on such as invalid_ok -- just to check if nothing is missing while allowing for errors in the value specification.
When we get to BIDS and if we decide to channel bids validator's errors similarly, such "bool flags" wouldn't be sufficient really.

@satra satra requested a review from yarikoptic June 29, 2021 00:08
@satra satra added patch Increment the patch version when merged release Create a release when this pr is merged labels Jun 30, 2021
@satra satra merged commit 28c13c3 into master Jun 30, 2021
@satra satra deleted the enh/utils branch June 30, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants