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

Properly set Config in create_model #320

Merged
merged 12 commits into from Dec 7, 2018

Conversation

2 participants
@hugoduncan
Copy link
Contributor

hugoduncan commented Dec 6, 2018

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

hugoduncan added some commits Dec 4, 2018

Properly set Config in create_model
Set the Config attribute in create_model, so it is found by the
MetaModel.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 6, 2018

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

samuelcolvin left a comment

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
@@ -100,17 +100,25 @@ def _extract_validators(namespace):
return validators


def inherit_validators(base_validators, validators):

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 6, 2018

Owner

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

@@ -100,17 +100,25 @@ def _extract_validators(namespace):
return validators


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

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 6, 2018

Owner

better to be explicit

if field in validators:
    validators[field] += field_validators
else:
    validators[field] = field_validators
annotations[f_name] = f_annotation
fields[f_name] = f_value

namespace = {"__annotations__": annotations, "__validators__": vg.validators}

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 6, 2018

Owner

single quotes please.

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

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 6, 2018

Owner

Is this definitely not required? if so please remove.

This comment has been minimized.

@hugoduncan

hugoduncan Dec 6, 2018

Contributor

I'll strip out some more incorrect validator logic.

ignore_extra = False
allow_extra = False

model = create_model("FooModel", foo=(int, ...), __config__=Config)

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 6, 2018

Owner

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.

This comment has been minimized.

@hugoduncan

hugoduncan Dec 6, 2018

Contributor

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

@samuelcolvin
Copy link
Owner

samuelcolvin left a comment

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

@@ -100,17 +100,30 @@ def _extract_validators(namespace):
return validators


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

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 7, 2018

Owner

if you want to do it this way

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

This comment has been minimized.

@hugoduncan

hugoduncan Dec 7, 2018

Contributor

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

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

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 7, 2018

Owner

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


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

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 7, 2018

Owner

validators =

@hugoduncan

This comment has been minimized.

Copy link
Contributor

hugoduncan commented Dec 7, 2018

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

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Dec 7, 2018

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

This comment has been minimized.

Copy link
Contributor

hugoduncan commented Dec 7, 2018

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?

@@ -353,6 +353,65 @@ def check_a_two(cls, v):
Child(a='snap')


def test_validate_parent_all():

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 7, 2018

Owner

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

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Dec 7, 2018

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

This comment has been minimized.

Copy link
Contributor

hugoduncan commented Dec 7, 2018

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

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Dec 7, 2018

my mistake, sorry.

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

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

3 checks passed

codecov/project 100% (+0%) compared to 86949c2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Dec 7, 2018

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

This comment has been minimized.

Copy link
Contributor

hugoduncan commented Dec 8, 2018

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

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Dec 8, 2018

Will do.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Dec 10, 2018

done.

@samuelcolvin samuelcolvin referenced this pull request Dec 28, 2018

Closed

Restore environment variable override behaviour #341

4 of 5 tasks complete

@samuelcolvin samuelcolvin referenced this pull request Jan 10, 2019

Open

Refactored extra types to use a single enum #352

1 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment