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

Support manually specifying case labels for union validators #841

Merged
merged 7 commits into from
Aug 15, 2023

Conversation

dmontagu
Copy link
Collaborator

@dmontagu dmontagu commented Jul 28, 2023

Produces error locs for union cases following the description in this comment: pydantic/pydantic#6897 (comment)

Also adds the ability to (optionally) specify the label to use for each union case.

I won't be offended if this gets rejected, or gets asked for some kind of rework.

Selected Reviewer: @davidhewitt

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #841 (40a5f7e) into main (7d67b91) will increase coverage by 0.06%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #841      +/-   ##
==========================================
+ Coverage   93.85%   93.91%   +0.06%     
==========================================
  Files         104      104              
  Lines       15387    15414      +27     
  Branches       25       25              
==========================================
+ Hits        14441    14476      +35     
+ Misses        940      932       -8     
  Partials        6        6              
Files Changed Coverage Δ
python/pydantic_core/core_schema.py 96.77% <100.00%> (ø)
src/serializers/type_serializers/union.rs 82.44% <100.00%> (+0.69%) ⬆️
src/validators/union.rs 85.80% <100.00%> (+4.10%) ⬆️

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@codspeed-hq
Copy link

codspeed-hq bot commented Jul 28, 2023

CodSpeed Performance Report

Merging #841 will degrade performances by 24.34%

Comparing improved-locs (40a5f7e) with main (7d67b91)

Summary

❌ 1 regressions
✅ 137 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main improved-locs Change
test_decimal_from_string_core 53.4 µs 70.5 µs -24.34%

@adriangb
Copy link
Member

My only complaint is the performance hit. I wonder if a Vec<(CombinedValidator, Option<String>)> would be better?

@dmontagu
Copy link
Collaborator Author

dmontagu commented Aug 8, 2023

@adriangb what do you think now? That updated-to-40% threshold might be helping... 😈 But looking at the raw report it doesn't seem that obviously bad. Hard to tell I guess 🤷‍♂️.

@davidhewitt
Copy link
Contributor

I will run a full local benchmark tomorrow and try to upload something useful.

@davidhewitt davidhewitt self-assigned this Aug 9, 2023
@davidhewitt
Copy link
Contributor

Had a quick chat with @samuelcolvin this morning and the impression I got was that he has opinions, I'm going to reassign this.

@dmontagu dmontagu changed the title Use new format for union/tagged-union error locs Support manually specifying case labels for union validators Aug 15, 2023
@dmontagu
Copy link
Collaborator Author

please review

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@davidhewitt davidhewitt enabled auto-merge (squash) August 15, 2023 14:51
@davidhewitt davidhewitt merged commit 09f0acf into main Aug 15, 2023
29 of 30 checks passed
@davidhewitt davidhewitt deleted the improved-locs branch August 15, 2023 15:00
dapper91 added a commit to dapper91/pydantic-xml that referenced this pull request Aug 22, 2023
dapper91 added a commit to dapper91/pydantic-xml that referenced this pull request Aug 22, 2023
- union schema choices type check added. Affected by pydantic/pydantic-core#841.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants