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

Perform validation on assignment to attribute #94

Merged
merged 4 commits into from Oct 31, 2017

Conversation

2 participants
@petroswork
Contributor

petroswork commented Oct 25, 2017

  • Add config variable "validate_assignment" defaulting to False.
  • Add unit test.
  • #92
Perform validation on assignment to attribute
* Add config variable "validate_assignment" defaulting to False.
* Add unit test.
@codecov

This comment has been minimized.

codecov bot commented Oct 25, 2017

Codecov Report

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

@@          Coverage Diff          @@
##           master    #94   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          11     11           
  Lines         821    827    +6     
  Branches      182    184    +2     
=====================================
+ Hits          821    827    +6

@petroswork petroswork changed the title from Perform validation on assignment to attribute to Perform validation on assignment to attribute #92 Oct 25, 2017

@petroswork petroswork changed the title from Perform validation on assignment to attribute #92 to Perform validation on assignment to attribute Oct 25, 2017

@petroswork

This comment has been minimized.

Contributor

petroswork commented Oct 25, 2017

I'm not certain what the second parameter of validate is for, so I left it empty.

@samuelcolvin

Thanks so much for your contribution, but a few things to fix here.

By the way, you don't need to delete your pull request and create another one to make an update. Instead just push more commits to the branch which you used for this pull request (in this case your master).

@@ -111,7 +112,13 @@ def __setattr__(self, name, value):
raise ValueError(f'"{self.__class__.__name__}" object has no field "{name}"')
elif not self.config.allow_mutation:
raise TypeError(f'"{self.__class__.__name__}" is immutable and does not support item assignment')
self.__values__[name] = value
if self.config.validate_assignment:

This comment has been minimized.

@samuelcolvin
value_, error_ = self.fields[name].validate(value, {})
if error_:
raise ValidationError({name: error_})
self.__values__[name] = value_

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 25, 2017

Owner

please use

if error_:
    raise ValidationError({name: error_})
else:
    self.__values__[name] = value_

I know the else is not required but I think it makes the code more readable.

def test_validating_assignment():
import pydantic

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 25, 2017

Owner

you don't need this import

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 25, 2017

Owner

So add from pydantic import constr to the top of the file.

def test_validating_assignment():
import pydantic
class Pedantic(BaseModel):

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 25, 2017

Owner

use ValidateAssignmentModel as the name here and move outside the test so it can be used in multiple tests.

p.b = "hi"
assert p.b == "hi"
assert p.values() == {"a": 2, "b": "hi"}
with pytest.raises(ValidationError) as exc_info:

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 25, 2017

Owner

split the failure into a separate test.

class Config:
validate_assignment = True
p = Pedantic(a=5, b="hello")

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 25, 2017

Owner

use single quotes for strings unless they include single quotes.

p.b = ""
assert "error validating input" in str(exc_info)
class Permissive(BaseModel):

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 25, 2017

Owner

move all this into separate tests.

@@ -111,7 +112,13 @@ def __setattr__(self, name, value):
raise ValueError(f'"{self.__class__.__name__}" object has no field "{name}"')
elif not self.config.allow_mutation:
raise TypeError(f'"{self.__class__.__name__}" is immutable and does not support item assignment')
self.__values__[name] = value
if self.config.validate_assignment:
value_, error_ = self.fields[name].validate(value, {})

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 25, 2017

Owner

the second argument to validate() should be the other values on the modal in case validation of this field needs to see these values. Here I guess you should use self.values(exclude={name}).

You should also add a test for this case.

Add non-empty second parameter to fields.validate
* Improved tests per maintainer's suggestions.
@petroswork

This comment has been minimized.

Contributor

petroswork commented Oct 26, 2017

The second parameter to fields.validate does not seem to work, at least not in the way I have in mind, hence the commented out test. Any suggestions?

@samuelcolvin

Nearly there!

We also need to add a section to docs and a line to HISTORY.rst. If that's too boring, I'll do it once this is ready and we've merged it.

class ValidateAssignmentModel(BaseModel):
a: int = 2
b: constr(min_length=1)
c: constr(min_length=a)

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 26, 2017

Owner

This doesn't make sense. Remove c completely.

As per #29 I need to improve how validators work, so I guess it's not really possible to test this case. Sorry.

This comment has been minimized.

@samuelcolvin
validate_assignment = True
def test_validating_assignment():

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 26, 2017

Owner

Unit tests are supposed to test one thing, so if the test fails you know what part of the code has gone wrong.

This isn't always simple, but we can do better here. Split this into test_validating_assignment_pass and test_validating_assignment_fail.

assert p.values() == {'a': 2, 'b': 'hi', 'c': 'testing'}
with pytest.raises(ValidationError) as exc_info:
p.a = 'b'
assert 'error validating input' in str(exc_info)

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 26, 2017

Owner

almost all ValidationErrors will include 'error validating input' either remove this or check the inner error is right.

with pytest.raises(ValidationError) as exc_info:
p.c = 'c'
assert 'error validating input' in str(exc_info)
p.a = 100

This comment has been minimized.

@samuelcolvin

samuelcolvin Oct 26, 2017

Owner

I guess remove from here on, I'll add a test once I improve validators.

petroswork added some commits Oct 28, 2017

@samuelcolvin samuelcolvin merged commit fe80317 into samuelcolvin:master Oct 31, 2017

3 checks passed

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

This comment has been minimized.

Owner

samuelcolvin commented Oct 31, 2017

Awesome, thank you.

This was referenced Oct 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment