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

Use discriminated unions to provide more helpful error messages #245

Merged
merged 5 commits into from
Jul 10, 2024

Conversation

mvandenburgh
Copy link
Member

Fixes #244

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

This allows Pydantic to generate more useful validation errors.
@mvandenburgh mvandenburgh marked this pull request as draft June 21, 2024 16:13
Some test data was missing the `schemaKey` attribute for `contributor`.
@mvandenburgh mvandenburgh force-pushed the pydantic-discriminated-unions branch from 0ce951c to f248164 Compare June 21, 2024 16:17
Copy link

codecov bot commented Jun 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@             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 Coverage Δ
unittests 97.74% <100.00%> (+10.31%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mvandenburgh mvandenburgh force-pushed the pydantic-discriminated-unions branch from 17cea29 to 3480631 Compare June 21, 2024 16:35
@mvandenburgh
Copy link
Member Author

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

@mvandenburgh mvandenburgh force-pushed the pydantic-discriminated-unions branch from 3480631 to b90238f Compare June 24, 2024 14:33
@candleindark
Copy link
Member

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 #236, so the changes in JSON schemas from that PR are yet to be included in the latest published schemas.

@yarikoptic
Copy link
Member

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"!

@mvandenburgh mvandenburgh marked this pull request as ready for review July 10, 2024 19:25
@mvandenburgh
Copy link
Member Author

Is there something I need to add to the PR to bump the schema version?

@satra satra added minor Increment the minor version when merged patch Increment the patch version when merged release Create a release when this pr is merged and removed minor Increment the minor version when merged labels Jul 10, 2024
@satra
Copy link
Member

satra commented Jul 10, 2024

just added labels so we can release with this. see the consts.py file. that's where the schema version will need to be incremented for the test to pass.

@mvandenburgh
Copy link
Member Author

just added labels so we can release with this. see the consts.py file. that's where the schema version will need to be incremented for the test to pass.

Thanks, I bumped the version in consts.py.

@satra
Copy link
Member

satra commented Jul 10, 2024

@candleindark - could you please provide your feedback if any or approve this PR?

Copy link
Member

@candleindark candleindark left a comment

Choose a reason for hiding this comment

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

Everything looks fine to me.

@candleindark
Copy link
Member

Screenshot 2024-07-10 at 1 38 57 PM

Any idea about these tests? I can't locate them in the github workflow files. The only place we use macos-latest is in test-dandi-cli.yml, these tests don't seem to be defined by that file.

@satra
Copy link
Member

satra commented Jul 10, 2024

@candleindark - fixed the branch merge requirements. merging.

@satra satra merged commit 06c0ee3 into master Jul 10, 2024
76 checks passed
@satra satra deleted the pydantic-discriminated-unions branch July 10, 2024 21:07
@satra
Copy link
Member

satra commented Jul 10, 2024

thanks @mvandenburgh for this PR. will you update the server to this release schema and python library?

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.

Use discriminated unions to improve validation errors
4 participants