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
Validators for dataclasses #454
Conversation
This would be great! Found this library today and couldn't understand why validators didn't work for dataclasses. Was this written in the docs and I missed this? |
The limitation is referred to in the docs: "Currently validators don’t work with dataclasses, if it’s something you want please create an issue on github". |
thanks @primal100, looks broadly good. I'll go through it in detail when I get a chance... manic week this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good but hard to fully review until the merge errors are resoved.
If you're not clear how to resolve this, you might be better off doing reset --soft
and create a new PR.
pydantic/fields.py
Outdated
values: Dict[str, Any], | ||
*, | ||
loc: 'LocType', | ||
cls: Optional[Type[Union['BaseModel', 'DataclassType']]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps you could use a named type Optional[Type[Union['BaseModel', 'DataclassType']]
as it's reused in quite a few places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a new ModelType
in types.py
is that ok?
pydantic/main.py
Outdated
model_name: str, | ||
*, | ||
__config__: Type[BaseConfig] = None, | ||
__base__: Type[BaseModel] = None, | ||
__module__: Optional[str] = None, | ||
__validators__: Mapping[str, classmethod] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we just use Dict
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed main.py
pydantic/utils.py
Outdated
return type(v) == type(ClassVar) and (sys.version_info < (3, 7) or getattr(v, '_name', None) == 'ClassVar') | ||
|
||
|
||
def is_classvar(ann_type: AnyType) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again this seems to be messed up due to a rebasing problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be ok now
pydantic/utils.py
Outdated
return _check_classvar(ann_type) or _check_classvar(getattr(ann_type, '__origin__', None)) | ||
|
||
|
||
def gather_validators(type_: AnyType) -> Dict[str, classmethod]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be in class_validators
, also can we use the same logic for models and dataclasses, without making it too complicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to class_validators.py
now.
The same logic for models (extract_validators
function ) is still used by dataclasses when the __pydantic_model__
is created. The new gather_validators
function just gets the validators in a format to where they can be added to the namespace in the create_model
function. Then the same logic as for BaseModel
is used when the Model is created.
Both extract_validators
and gather_validators
contain a check for the __validator_config
attribute so there is a slight duplication there. One alternative would be to add the entire namespace (__dict__
) of the dataclass to create_model
(instead of just validators) and then let the BaseModel
__new__
method filter the namespace for validators as normal. What do you think?
Codecov Report
@@ Coverage Diff @@
## master #454 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 14 14
Lines 2230 2238 +8
Branches 437 440 +3
=====================================
+ Hits 2230 2238 +8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a few small things and questions.
pydantic/main.py
Outdated
if __validators__: | ||
try: | ||
if any(not hasattr(v, '__validator_config') for k, v in __validators__.items()): | ||
raise ConfigError('Validator methods must be decorated with @validator') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really required?
We don't inspect all arguments to all functions, if we were doing that we might as well right rust.
I think better to remove, but if it's really required for some reason, it'll need tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, especially with type checking it's not needed. Removed these lines.
pydantic/types.py
Outdated
from .utils import AnyCallable | ||
|
||
CallableGenerator = Generator[AnyCallable, None, None] | ||
ModelType = Optional[Type[Union['BaseModel', 'DataclassType']]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is deceptive, better to have
ModelOrDc = Type[Union['BaseModel', 'DataclassType']]
So the name is clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, but set to:
ModelOrDc = Optional[Type[Union['BaseModel', 'DataclassType']]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to create without Optional? And then just write Optional[ModelOrDc] where it is used? What do you think? I can easily make the change if required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please, then it's clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
HISTORY.rst
Outdated
@@ -14,6 +14,7 @@ v0.23 (2019-04-04) | |||
* Support specialized ``ClassVars``, #455 by @tyrylu | |||
* fix JSON serialization for ``ipaddress`` types, #333 by @pilosus | |||
* add ``SecretStr`` and ``SecretBytes`` types, #452 by @atheuz | |||
* Support validators in dataclasses, #454 by @primal100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong place version is released, Please start a new section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to latest section in scolvin:master
tests/test_validators_dataclass.py
Outdated
class Child(Parent): | ||
pass | ||
|
||
assert Parent(a='this is foobar good').a == 'this is foobar good' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more concise to have rename check_a
to change_a
, and return v + 'changed'
, then you can have just these two lines.
Can you also add a test with a validator having the same name as in the parent, does it overwrite, call both, or raise an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The child validator overwrites the parent(same behavior as BaseModel)
Getting an error when running "make":
Also gives an error on the Docs build:
However, I made a clone of the samuelcolvin:master branch just to check and see the same error when I run make |
My fault, I made a mistake when adding a line to HISTORY earlier, should be fixed now. |
awesome, thank you so much. |
Change Summary
Custom validator methods work with dataclasses
Related issue number
#415: Validator on dataclasses
Also remove a limitation referred to in the docs
Checklist
HISTORY.rst
has been updated#<number>
@<whomever>
There are probably a few decisions to be made with regard to the exact implementation. Maybe some of this can be improved?
The validators are discovered using a new utils function, gather_validators, which looks for validator methods in the dataclass and parent classes
The create_model function now accepts a validators keyword argument to pass in a mapping of method names and methods. These are then added to the namespace by the create_model function.
The validate_model now accepts an additional cls keyword argument. The pydantic_model already gets passed in from the dataclass, so the additional cls argument is also added so that the correct class (the dataclass) is passed to the validator classmethods.
The example datetime.py was renamed to datetime_example.py as the examples which import datetime were failing to run due to the name clash