Skip to content
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

Add better support for floating point multiple_of values #652

Merged

Conversation

@justindujardin
Copy link
Contributor

commented Jul 10, 2019

Change Summary

Add support for multiple_of with arbitrary int/floating point numbers. The current implementation fails for obvious floating point values, e.g. 0.3 fails to validate as a multiple of 0.1.

Related issue number

None

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where 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>
    • include your github username @<whomever>

@justindujardin justindujardin force-pushed the justindujardin:bugfix/float_multiple_of branch from 869e189 to 05b2307 Jul 10, 2019

@codecov

This comment has been minimized.

Copy link

commented Jul 10, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #652   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2651   2655    +4     
  Branches      524    526    +2     
=====================================
+ Hits         2651   2655    +4

@justindujardin justindujardin force-pushed the justindujardin:bugfix/float_multiple_of branch from 05b2307 to 266788b Jul 10, 2019

@justindujardin justindujardin changed the title :bug: fix for floating point multiple_of values Add better support for floating point multiple_of values Jul 10, 2019

pydantic/validators.py Show resolved Hide resolved
tests/test_types.py Outdated Show resolved Hide resolved
@justindujardin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

While working on this I noticed that JSONSchema spec says mutliple_of must strictly be greater than 0.0, so I added a validation error and a test for this behavior.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jul 11, 2019

You're checking v not multiple_of.

Also your calling float twice.

@justindujardin justindujardin force-pushed the justindujardin:bugfix/float_multiple_of branch from b16bf6c to 6b89230 Jul 11, 2019

@justindujardin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Oof, yeah. Is there a more appropriate place to check this then? Somewhere maybe it could be an error during declaration rather than at validation time?

I reverted the extra validation commit for the time being.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jul 11, 2019

(on my mobile)

Yes, they're better places, probably in validators.py.

@justindujardin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

@samuelcolvin FYI I'm not planning on following up with the > 0 assertion in this PR. If there's anything else you would like changed to support floating point multiple_of let me know. Thanks for your review!

@tiangolo
Copy link
Collaborator

left a comment

LGTM 🎉

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Jul 15, 2019

LGTM, @justindujardin please could you rebase and move the new history line to the new section.

I'm aware the history thing is getting annoying I'll try to adopt towncrier or similar soon.

Add better support for floating point multiple_of values
 - modulo doesn't work with floating point values in many cases, e.g. `0.3 % 0.1 == 0.09999999999999998`
 - port implementation from: tdegrunt/jsonschema#187 (comment)
 - add tests for int/float multiple_of values
 - update history with pr/author

@justindujardin justindujardin force-pushed the justindujardin:bugfix/float_multiple_of branch from 6b89230 to de88ddf Jul 15, 2019

@justindujardin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@samuelcolvin sure thing, done

@samuelcolvin samuelcolvin merged commit bc60014 into samuelcolvin:master Jul 23, 2019

6 of 9 checks passed

Header rules No header rules processed
Details
Pages changed 2 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190715.11 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details

@justindujardin justindujardin deleted the justindujardin:bugfix/float_multiple_of branch Jul 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.