Skip to content

fix: submodel fields with wrap validator affect smart union selection #1700

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

weiliddat
Copy link

@weiliddat weiliddat commented Apr 18, 2025

Change Summary

  • Keep fields_set_count state when validating submodels

Related issue number

Fixes pydantic/pydantic#11752

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @davidhewitt

Comment on lines +289 to +290
// state.exactness = self.exactness;
// state.fields_set_count = self.fields_set_count;
Copy link
Author

Choose a reason for hiding this comment

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

@Viicos I'm not sure that these are being used at the moment for validate_assignment. I don't see any explicit use for these — I've also added an additional assertion in the added test.

But maybe Chesterton's Fence in effect 😄

Let me know if I should keep it

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch, I'm tempted to leave it and add a TODO is it necessary to report the state for validate_assignment?, but is this TODO going to be solved anytime.. 😄 thoughts @davidhewitt?

Copy link
Author

Choose a reason for hiding this comment

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

I'll dig into it a bit more to see if this state is necessary on validate_assignment, but if I can't figure it out this week then I'll just add the TODO and leave it

@weiliddat
Copy link
Author

please review

Copy link

codspeed-hq bot commented Apr 18, 2025

CodSpeed Performance Report

Merging #1700 will not alter performance

Comparing weiliddat:fix/wrap-validator-smart-union (0bb8a07) with main (3414703)

Summary

✅ 157 untouched benchmarks

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.

WrapValidator annotation changes smart union score
3 participants