search

dandi / dandi-schema

Schemata for DANDI archive project
Apache License 2.0
5 stars 8 forks source link

Use discriminated unions to provide more helpful error messages #245

Open mvandenburgh opened 2 weeks ago

mvandenburgh commented 2 weeks ago

Fixes #244

This change allows Pydantic to generate more useful validation errors by making use of discriminated unions where possible.

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.74%. Comparing base (e135307) to head (a23a0a9).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #245 +/- ## =========================================== + Coverage 87.42% 97.74% +10.31% =========================================== Files 16 16 Lines 1726 1727 +1 =========================================== + Hits 1509 1688 +179 + Misses 217 39 -178 ``` | [Flag](https://app.codecov.io/gh/dandi/dandi-schema/pull/245/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dandi/dandi-schema/pull/245/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi) | `97.74% <100.00%> (+10.31%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mvandenburgh commented 2 weeks ago

There's some test failures happening here that I have to look into, marking this as draft for now.

candleindark commented 4 days ago

The changes look good to me.

The JSON schemas have changed though mostly because of the inclusion of the discriminator object as mentioned in the discriminated union docs. We may need to increase the JSON schema version, but I don't expect much perceivable difference for the end users.

@yarikoptic @satra Should me up the JSON schema version for this one? We didn't up the JSON schema version for https://github.com/dandi/dandi-schema/pull/236, so the changes in JSON schemas from that PR are yet to be included in the latest published schemas.

yarikoptic commented 3 days ago

my personal take is that for any change upon release we should up the json version, otherwise it all becomes ambiguous and possibly hard to troubleshoot later on. "explicit better than implicit"!