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

use type() in int validator #264

Merged
merged 4 commits into from Sep 21, 2018

Conversation

2 participants
@nphyatt
Contributor

nphyatt commented Sep 19, 2018

Change Summary

This fixes an issue I would not expect where if you pass True | False to and field typed with int you'll get the bool value inside your object. I would expect to get int values 1 | 0 This is because in python a bool is actually a subclass of int so isinstance(True, int) returns True. Using type(True) is int returns False and will therefore cause True to be cast to an int which I think falls in line with how the other validators work.

Related issue number

I didn't see any.

Performance Changes

pydantic cares about performance, if there's any risk performance changed on this PR,
please run make benchmark-pydantic before and after the change:

  • before:
pydantic time=0.962s, success=48.10%
pydantic time=0.987s, success=48.10%
pydantic time=0.988s, success=48.10%
pydantic time=0.932s, success=48.10%
pydantic time=1.003s, success=48.10%
pydantic best=0.932s, avg=0.975s, stdev=0.028s

pydantic best=31.082μs/iter avg=32.493μs/iter stdev=0.930μs/iter
  • after:
pydantic time=0.951s, success=48.10%
pydantic time=0.970s, success=48.10%
pydantic time=0.963s, success=48.10%
pydantic time=0.930s, success=48.10%
pydantic time=0.971s, success=48.10%
pydantic best=0.930s, avg=0.957s, stdev=0.017s

pydantic best=31.000μs/iter avg=31.900μs/iter stdev=0.566μs/iter

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
@codecov

This comment has been minimized.

codecov bot commented Sep 19, 2018

Codecov Report

Merging #264 into master will decrease coverage by 18.85%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master     #264       +/-   ##
===========================================
- Coverage     100%   81.14%   -18.86%     
===========================================
  Files          11       11               
  Lines        1480     1824      +344     
  Branches      274      274               
===========================================
  Hits         1480     1480               
- Misses          0      344      +344
@samuelcolvin

This comment has been minimized.

Owner

samuelcolvin commented Sep 20, 2018

Looks good, my only question is what happens with the following:

class MyInt(int):
    pass

class FooModel(BaseModel):
   v: MyInt

I would expect it to be validated as an int. More dangerous: people could already have code like this.

Maybe safter to have:

def int_validator(v) -> int:
    if not isinstance(v, bool) and isinstance(v, int):
        return v

?

@nphyatt

This comment has been minimized.

Contributor

nphyatt commented Sep 21, 2018

Good call PR updated.

@samuelcolvin samuelcolvin merged commit 10414a7 into samuelcolvin:master Sep 21, 2018

2 checks passed

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 Sep 21, 2018

thanks a lot, will do a release now.

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