-
-
Notifications
You must be signed in to change notification settings - Fork 11
feat: combine multipleOf errors into a single message
#150
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
feat: combine multipleOf errors into a single message
#150
Conversation
jdesrosiers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start. I've asked for a couple refinements inline.
src/test-suite/tests/multipleOf.json
Outdated
| "errors": [] | ||
| }, | ||
| { | ||
| "description": "multiple multipleOf constraints (combined)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good test! But, this needs a better name. This is checking the LCM logic right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why'd you delete this test? This was a necessary test for the LCM logic. It just needed a better name. The other test is not sufficient to test LCM. An implementation that naively multiplied the value of all multipleOf values would pass that test. This test would catch that bug.
|
Hii @jdesrosiers,
Please let me know if you’d like any further changes. |
|
@jdesrosiers I've added the test with |
|
Just the one test might be enough. I can't think of a case where the 2-value case would fail but the 3-value would pass. I think just the three-value case would be fine. |
|
I’ve kept only the three-value test case ( |
jdesrosiers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Combine multiple failing
multipleOfconstraints at the same instance locationinto a single error message by computing their LCM. Tests included.
Closes #139