Skip to content

Conversation

nphyatt
Copy link
Contributor

@nphyatt 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
Copy link

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

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
Copy link
Contributor Author

nphyatt commented Sep 21, 2018

Good call PR updated.

@samuelcolvin samuelcolvin merged commit 10414a7 into pydantic:master Sep 21, 2018
@samuelcolvin
Copy link
Member

thanks a lot, will do a release now.

alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
* Switched order of strip_whitespace to before length checks

* Changed logic of string length test

* Changed pattern test to match whitespace stripping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants