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

Fix @model_validator on nested models #5889

Merged
merged 12 commits into from
May 27, 2023
Merged

Conversation

adriangb
Copy link
Member

@adriangb adriangb commented May 26, 2023

Fixes #5790

Selected Reviewer: @Kludex

@adriangb
Copy link
Member Author

adriangb commented May 26, 2023

TODO: need test for @model_serializer done

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 26, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 32a1b9e
Status: ✅  Deploy successful!
Preview URL: https://940fecf5.pydantic-docs2.pages.dev
Branch Preview URL: https://fix-bug-nested-model-validat.pydantic-docs2.pages.dev

View logs

@adriangb adriangb force-pushed the fix-bug-nested-model-validator branch from 0f73ca7 to 32ee89c Compare May 26, 2023 18:08
@adriangb
Copy link
Member Author

please review

@@ -181,7 +181,7 @@ def model_serializer(
*,
mode: Literal['plain', 'wrap'] = 'plain',
json_return_type: _core_schema.JsonReturnTypes | None = None,
) -> Callable[[Any], _decorators.PydanticDescriptorProxy[Any]] | _decorators.PydanticDescriptorProxy[Any]:
) -> Callable[[Any], Any]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a problem that the non-callable return type was removed here? I mean, I'm guessing it's not a problem, but it would it be more careful in some way? Happy to leave like this if preferable just want to make sure it wasn't an oversight.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed a slightly better version. We can get even more granular (e.g. defining a couple of Protocols like we do form some of the other validators/serializers) but I don't want to do that in this PR, I'm only changing it because it was causing issues with the test I'm adding here.

Copy link
Member

Choose a reason for hiding this comment

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

pls can we add (if we don't already have) an example of both @model_validator and @model_validator(...) in mypy tests please.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM, tests seem to be failing

@@ -181,7 +181,7 @@ def model_serializer(
*,
mode: Literal['plain', 'wrap'] = 'plain',
json_return_type: _core_schema.JsonReturnTypes | None = None,
) -> Callable[[Any], _decorators.PydanticDescriptorProxy[Any]] | _decorators.PydanticDescriptorProxy[Any]:
) -> Callable[[Any], Any]:
Copy link
Member

Choose a reason for hiding this comment

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

pls can we add (if we don't already have) an example of both @model_validator and @model_validator(...) in mypy tests please.

@dmontagu dmontagu dismissed samuelcolvin’s stale review May 27, 2023 00:00

Added @model_validator to mypy test module

@adriangb adriangb merged commit 742c391 into main May 27, 2023
52 checks passed
@adriangb adriangb deleted the fix-bug-nested-model-validator branch May 27, 2023 00:08
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.

model_validator not called for nested Models
4 participants