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.

Sign up for GitHub

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

Jump to bottom

Add better support for floating point multiple_of values #652

Merged
merged 1 commit into from Jul 23, 2019
Merged

Add better support for floating point multiple_of values #652

merged 1 commit into from Jul 23, 2019

Conversation

justindujardin
Copy link
Contributor

@justindujardin justindujardin commented Jul 10, 2019 •
edited

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>

@codecov
Copy link

codecov bot commented Jul 10, 2019 •
edited

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 changed the title 🐛 fix for floating point multiple_of values Add better support for floating point multiple_of values Jul 10, 2019
samuelcolvin
samuelcolvin reviewed Jul 11, 2019
View reviewed changes
pydantic/validators.py Show resolved Hide resolved
tests/test_types.py Outdated Show resolved Hide resolved
@justindujardin
Copy link
Contributor Author

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

You're checking v not multiple_of.

Also your calling float twice.

@justindujardin
Copy link
Contributor Author

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

(on my mobile)

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

@justindujardin
Copy link
Contributor Author

@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
tiangolo approved these changes Jul 15, 2019
View reviewed changes
Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@samuelcolvin
Copy link
Member

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.

@justindujardin
Add better support for floating point multiple_of values
de88ddf 
 - 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
Copy link
Contributor Author

@samuelcolvin sure thing, done

@samuelcolvin samuelcolvin merged commit bc60014 into pydantic:master Jul 23, 2019
@justindujardin justindujardin deleted the bugfix/float_multiple_of branch July 23, 2019 18:50
alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
@adriangb
Move dataclass config onto dataclass schema (pydantic#652)
ca48482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

@tiangolo tiangolo tiangolo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@justindujardin @samuelcolvin @tiangolo