Teach mypy plugin that validator methods are classmethods#4102
Merged
samuelcolvin merged 5 commits intopydantic:masterfrom Aug 9, 2022
Merged
Teach mypy plugin that validator methods are classmethods#4102samuelcolvin merged 5 commits intopydantic:masterfrom
samuelcolvin merged 5 commits intopydantic:masterfrom
Conversation
For your consideration: a patch which implements the suggestion I made here: pydantic#4101 Briefly: pydantic automatically wraps validator methods using `@classmethod`. Hence the first argument to a user-provided validator should be `cls`. But mypy doesn't know this: it analyses validator methods like any other regular method, believing the first parameter `cls` to be a model instance (usually denoted self). This means that if one annotates `cls` as `Type[...]` then mypy believes raises an error: error: The erased type of self "Type[temp.Model]" is not a supertype of its class "temp.Model" I concede that this is an extremely niche thing. The only tangible end-user benefit I can think of is that it'll stop you from calling instance methods in a validator. ---- I haven't written a mypy plugin before, so this is a bit of a hack-until-it-works. But it was more straightforward than I expected to get something working!
Contributor
Author
|
please review |
samuelcolvin
reviewed
Aug 8, 2022
Member
samuelcolvin
left a comment
There was a problem hiding this comment.
Could you also add a failing example to the tests.
| name: str | ||
|
|
||
| @validator('name') | ||
| def noop_validator_with_annotations(cls: Type['ModelWithAnnotatedValidator'], name: str) -> str: |
Member
There was a problem hiding this comment.
I think this is wrong, surely (like self) you can omit the type hint for cls?
Contributor
Author
There was a problem hiding this comment.
Ahh yes. I think I'd put this in so that the test failed without the change. Let me see if I can come up with something better.
Member
|
otherwise LGTM. Please update. Also conflicts. |
Contributor
Author
|
Many thanks for reviewing. I've merged in master and updated the tests. Please review. |
Member
|
thanks so much. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For your consideration: a patch which implements the suggestion I made here:
#4101
Briefly: pydantic automatically wraps validator methods using
@classmethod. Hence the first argument to a user-provided validatorshould be
cls. But mypy doesn't know this: it analyses validatormethods like any other regular method, believing the first parameter
clsto be a model instance (usually denoted self).This means that if one annotates
clsasType[...]then mypybelieves raises an error:
I concede that this is an extremely niche thing. The only tangible
end-user benefit I can think of is that it'll stop you from calling
instance methods in a validator.
I haven't written a mypy plugin before, so this is a bit of a
hack-until-it-works. But it was more straightforward than I expected to
get something working!
Change Summary
Related issue number
fix #4101
Checklist
changes/<pull request or issue id>-<github username>.mdfile added describing change(see changes/README.md for details)