Skip to content

Fix multipleOf constraint for integers #19

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mittsh
Copy link

@mittsh mittsh commented Mar 2, 2017

Proposal to fix the multipleOf constraint for integers. In fact, it wasn't working so far for the type integer. If the value passed was a Swift Int, we got value as? Double being evaluated to nil.

I made some changes, to try to coherce both number and value into an Int to use the standard modulo operator. And fallback on floating point comparison.

Also, I've replaced the previous flooring method by truncatingRemainder(dividingBy:) for floating point division.

Let me know what you think @kylef I'm happy to make any changes if coding style or implementation isn't matching the current project standards. Thanks in advance!

@kylef
Copy link
Owner

kylef commented Mar 6, 2017

Looks like these changes are breaking one of the tests (https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/8f867168d17497a775141704960906db83634a04/tests/draft4/multipleOf.json#L48-L52)

JSONSchemaTests.JSONSchemaCases
  testEverything, XCTAssertEqual failed: ("false") is not equal to ("true") - Failed validation: ["0.0075 is not a multiple of 0.0001"]
  /Users/travis/build/kylef/JSONSchema.swift/Tests/JSONSchemaCases.swift:113

      case .invalid(let errors):
        XCTAssertEqual(result.valid, test.value, "Failed validation: \(errors)")
      }

Could you take a look into this please?

@mittsh
Copy link
Author

mittsh commented Mar 6, 2017

Absolutely, yes. I'll have a look. Thank you! 😄

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