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

Validators for dataclasses #454

Merged
merged 28 commits into from Apr 11, 2019
Merged

Validators for dataclasses #454

merged 28 commits into from Apr 11, 2019

Conversation

@primal100
Copy link
Contributor

@primal100 primal100 commented Apr 2, 2019

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

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100% 99.3% some tests skipped
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<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

@Ronserruya
Copy link

@Ronserruya Ronserruya commented Apr 4, 2019

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?

Loading

@primal100
Copy link
Contributor Author

@primal100 primal100 commented Apr 4, 2019

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".

Loading

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Apr 4, 2019

thanks @primal100, looks broadly good. I'll go through it in detail when I get a chance... manic week this week.

Loading

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

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.

Loading

HISTORY.rst Show resolved Hide resolved
Loading
values: Dict[str, Any],
*,
loc: 'LocType',
cls: Optional[Type[Union['BaseModel', 'DataclassType']]] = None,
Copy link
Owner

@samuelcolvin samuelcolvin Apr 8, 2019

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.

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 10, 2019

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?

Loading

pydantic/main.py Outdated
model_name: str,
*,
__config__: Type[BaseConfig] = None,
__base__: Type[BaseModel] = None,
__module__: Optional[str] = None,
__validators__: Mapping[str, classmethod] = None,
Copy link
Owner

@samuelcolvin samuelcolvin Apr 8, 2019

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?

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 10, 2019

Choose a reason for hiding this comment

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

Changed main.py

Loading

return type(v) == type(ClassVar) and (sys.version_info < (3, 7) or getattr(v, '_name', None) == 'ClassVar')


def is_classvar(ann_type: AnyType) -> bool:
Copy link
Owner

@samuelcolvin samuelcolvin Apr 8, 2019

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.

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 10, 2019

Choose a reason for hiding this comment

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

Should be ok now

Loading

tests/test_validators_dataclass.py Show resolved Hide resolved
Loading
return _check_classvar(ann_type) or _check_classvar(getattr(ann_type, '__origin__', None))


def gather_validators(type_: AnyType) -> Dict[str, classmethod]:
Copy link
Owner

@samuelcolvin samuelcolvin Apr 8, 2019

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?

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 10, 2019

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?

Loading

@samuelcolvin samuelcolvin mentioned this pull request Apr 8, 2019
4 tasks
@codecov
Copy link

@codecov codecov bot commented Apr 10, 2019

Codecov Report

Merging #454 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #454   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        2230   2238    +8     
  Branches      437    440    +3     
=====================================
+ Hits         2230   2238    +8

Loading

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Looking good, just a few small things and questions.

Loading

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')
Copy link
Owner

@samuelcolvin samuelcolvin Apr 11, 2019

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.

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 11, 2019

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.

Loading

pydantic/main.py Show resolved Hide resolved
Loading
from .utils import AnyCallable

CallableGenerator = Generator[AnyCallable, None, None]
ModelType = Optional[Type[Union['BaseModel', 'DataclassType']]]
Copy link
Owner

@samuelcolvin samuelcolvin Apr 11, 2019

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.

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 11, 2019

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']]]

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 11, 2019

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.

Loading

Copy link
Owner

@samuelcolvin samuelcolvin Apr 11, 2019

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.

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 11, 2019

Choose a reason for hiding this comment

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

Done!

Loading

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
Copy link
Owner

@samuelcolvin samuelcolvin Apr 11, 2019

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.

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 11, 2019

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

Loading

class Child(Parent):
pass

assert Parent(a='this is foobar good').a == 'this is foobar good'
Copy link
Owner

@samuelcolvin samuelcolvin Apr 11, 2019

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?

Loading

Copy link
Contributor Author

@primal100 primal100 Apr 11, 2019

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)

Loading

@primal100
Copy link
Contributor Author

@primal100 primal100 commented Apr 11, 2019

Getting an error when running "make":

warning: check: Duplicate explicit target name: "@atheuz".

Also gives an error on the Docs build:

/opt/build/repo/docs/.TMP_HISTORY.rst:4:Duplicate explicit target name: "@atheuz".

However, I made a clone of the samuelcolvin:master branch just to check and see the same error when I run make

Loading

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Apr 11, 2019

Getting an error when running "make":

warning: check: Duplicate explicit target name: "@atheuz".

Also gives an error on the Docs build:

/opt/build/repo/docs/.TMP_HISTORY.rst:4:Duplicate explicit target name: "@atheuz".

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.

Loading

@samuelcolvin samuelcolvin merged commit 50fd2c5 into samuelcolvin:master Apr 11, 2019
6 checks passed
Loading
@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Apr 11, 2019

awesome, thank you so much.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants