Properly set Config in create_model by hugoduncan · Pull Request #320 · pydantic/pydantic

hugoduncan · 2018-12-06T05:16:49Z

Change Summary

Set the Config attribute in create_model, so it is found by the
MetaModel. Pushes the field and validator configuration into the
metaclass.

Related issue number

None

Performance Changes

Not run.

Checklist

Unit tests for the changes exist
Tests pass on CI and coverage remains at 100%
Documentation reflects the changes
No performance deterioration (if 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>
- if you're not a regular contributer please include your github username @whatever

Set the Config attribute in create_model, so it is found by the MetaModel.

codecov · 2018-12-06T05:24:43Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #320   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines        1761   1771   +10     
  Branches      340    344    +4     
=====================================
+ Hits         1761   1771   +10

samuelcolvin

Thanks for the changes, overall this looks like a good idea. Thanks.

There are a few changes here that require tests:

validator inheritance in the standard model inheritance case, with and without further subsequent validators on the same field in inheriting models
validators in create_model

samuelcolvin · 2018-12-06T10:55:28Z

    return validators


+def inherit_validators(base_validators, validators):


better if this method returns validators so it's clear what's going on.

samuelcolvin · 2018-12-06T10:56:09Z


+def inherit_validators(base_validators, validators):
+    for field, field_validators in base_validators.items():
+        validators.setdefault(field, []).extend(field_validators)


better to be explicit

if field in validators: validators[field] += field_validators else: validators[field] = field_validators

samuelcolvin · 2018-12-06T10:57:22Z

+            annotations[f_name] = f_annotation
+        fields[f_name] = f_value
+
+    namespace = {"__annotations__": annotations, "__validators__": vg.validators}


single quotes please.

samuelcolvin · 2018-12-06T10:59:51Z

-    namespace = {'config': config, '__fields__': fields}
+        # f_validators = vg.get_validators(f_name)
+        # if f_validators:
+        #     setattr(f_value, "__validator_config", f_validators)


Is this definitely not required? if so please remove.

I'll strip out some more incorrect validator logic.

samuelcolvin · 2018-12-06T11:00:35Z

+        ignore_extra = False
+        allow_extra = False
+
+    model = create_model("FooModel", foo=(int, ...), __config__=Config)


I haven't got around to finding a way to lint this, but pydantic should use single quotes always unless single quotes are used in the string.

We struggled with this with black as well. Ended up just using the Black convention.

samuelcolvin

Still needs the extra tests mentioned above, otherwise looking good.

samuelcolvin · 2018-12-07T08:55:44Z


+def inherit_validators(base_validators, validators):
+    for field, field_validators in base_validators.items():
+        if field in validators:


if you want to do it this way

if field not in validators: validators[field] = [] validators[field] += field_validators

This is to avoid mutating the validators in the base classes.

a = [] b = a b += [1] assert a == [1]

what I've suggested has exactly the same effect as what you have now, just avoids duplicated lines.

samuelcolvin · 2018-12-07T08:56:09Z


        config = inherit_config(namespace.get('Config'), config)
-        vg = ValidatorGroup(_extract_validators(namespace))
+        inherit_validators(_extract_validators(namespace), validators)


validators =

hugoduncan · 2018-12-07T14:47:21Z

@samuelcolvin I've extended the two existing validation test using inheritance. Is that sufficient or did you have something further in mind?

samuelcolvin · 2018-12-07T14:48:39Z

Definitely requires more tests, from above:

There are a few changes here that require tests:

validator inheritance in the standard model inheritance case, with and without further subsequent validators on the same field in inheriting models

validators in create_model

hugoduncan · 2018-12-07T16:26:01Z

I've added some tests for child classes using validators from parent classes, to complement the existing tests for adding validation in child classes.

validators in create_model

Do you have specific test cases in mind that aren't covered by test_inheritance_validators and test_inheritance_validators_all?

samuelcolvin · 2018-12-07T16:44:17Z

        Child(a='snap')


+def test_validate_parent_all():


looks like this test and the one at the bottom should swap name, so this becomes test_validate_child_all.

samuelcolvin · 2018-12-07T16:46:11Z

Do you have specific test cases in mind

yes, a test where we have a model with validators, then us that as a base in create_model and check the validators are called correctly on the created model.

I'm not saying it'll add any specific coverage (it can't since we have 100% coverage), but since we think that should be possible we should have a test of doing it.

hugoduncan · 2018-12-07T17:26:48Z

yes, a test where we have a model with validators, then us that as a base in create_model and check the validators are called correctly on the created model.

I've added some detail around the test_inheritance_validators test, adding a case for always=True. Otherwise I'm struggling to see how this doesn't cover what you describe.

samuelcolvin · 2018-12-07T18:03:22Z

my mistake, sorry.

LGTM, I'll look full and merge early next week.

samuelcolvin · 2018-12-07T20:28:22Z

great, thank you very much.

Will deploy in a week or two, unless you need it urgently in which case let me know and I'll deploy sooner.

hugoduncan · 2018-12-08T01:10:04Z

Thank you - the review process was definitely useful! If you could manage to release sooner it would save me the work of deploying an internal fork.

samuelcolvin · 2018-12-08T11:44:18Z

Will do.

samuelcolvin · 2018-12-10T10:07:49Z

done.

* uprev pyo3 to 0.17.3 * hopefully speed up ValidationError.errors()

hugoduncan added 2 commits December 6, 2018 00:12

Properly set Config in create_model

aac2416

Set the Config attribute in create_model, so it is found by the MetaModel.

Black formatting fixes

f31f096

samuelcolvin requested changes Dec 6, 2018

View reviewed changes

hugoduncan added 5 commits December 6, 2018 08:17

Remove uneeded validator logic

c5b04da

Use single quotes

e549ab7

Add return value to inherit_validators

69f9f95

Refactor inherit_validators

8112011

Add HISTORY.rst entry

f63e253

samuelcolvin reviewed Dec 7, 2018

View reviewed changes

hugoduncan added 2 commits December 7, 2018 08:55

Address feedback

6353a5a

Extend existing validator test cases and remove field aliasing

46e7559

Add test cases for validators inherted from parent

4810ab0

samuelcolvin reviewed Dec 7, 2018

View reviewed changes

hugoduncan added 2 commits December 7, 2018 11:52

Make test case names consistent with where validators are configured

ee567d9

Add detail create_model validator inheritance tests

457f40d

samuelcolvin merged commit 3249330 into pydantic:master Dec 7, 2018

This was referenced Dec 25, 2018

prevent validators being called repeatedly after inheritance #327

Merged

Dynamically generated models should inherit the Config of their __base__ #283

Closed

Restore environment variable override behaviour #341

Closed

samuelcolvin mentioned this pull request Jan 10, 2019

Refactored extra types to use a single enum #352

Merged

4 tasks

alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023

uprev pyo3 to 0.17.3 (pydantic#320)

791e06a

* uprev pyo3 to 0.17.3 * hopefully speed up ValidationError.errors()

		return validators


		def inherit_validators(base_validators, validators):

Uh oh!

Conversation

hugoduncan commented Dec 6, 2018 • edited by samuelcolvin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Summary

Related issue number

Performance Changes

Checklist

Uh oh!

codecov Bot commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

samuelcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugoduncan commented Dec 7, 2018

Uh oh!

samuelcolvin commented Dec 7, 2018

Uh oh!

hugoduncan commented Dec 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelcolvin commented Dec 7, 2018

Uh oh!

hugoduncan commented Dec 7, 2018

Uh oh!

samuelcolvin commented Dec 7, 2018

Uh oh!

samuelcolvin commented Dec 7, 2018

Uh oh!

hugoduncan commented Dec 8, 2018

Uh oh!

samuelcolvin commented Dec 8, 2018

Uh oh!

samuelcolvin commented Dec 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hugoduncan commented Dec 6, 2018 •

edited by samuelcolvin

Loading

codecov Bot commented Dec 6, 2018 •

edited

Loading