Update oas_models.py to support multipleOf#139
Update oas_models.py to support multipleOf#139kevinmcody wants to merge 1 commit intoMarketSquare:mainfrom
Conversation
Support multipleOf constraint on an Integer or Number. Strategy: Use the ceiling of Min divided by multipleOf, floor of Max divided by multipleOf, for the random min/max, then multiply by multipleOf
|
Thanks for looking into this @kevinmcody ! Looking into Considering the phrasing of the JSON specification, the annotation for Using Note that annotation of the Also I don´t check for min > max within the models. This is a deliberate choice; if the schema does not make sense, the request data will simply fail schema validation without the need of extra validations / checks within the models / value generation. The ToDo's for the Before this can be released, the tests in I've created a branch to continue the work on this feature: https://github.com/MarketSquare/robotframework-openapitools/tree/76_add_support_for_multipleof |
|
Thanks for looking it over.
I was also a little intimidated by the invalid values, that’s why I sheepishly left them as TODOs. 😊
I will certainly get on your suggested refactor, and look at the tests though, probably sometime this week.
From: Robin ***@***.***>
Sent: Monday, April 27, 2026 9:07 AM
To: MarketSquare/robotframework-openapitools ***@***.***>
Cc: Kevin Cody ***@***.***>; Mention ***@***.***>
Subject: Re: [MarketSquare/robotframework-openapitools] Update oas_models.py to support multipleOf (PR #139)
[Image removed by sender.]robinmackaij left a comment (MarketSquare/robotframework-openapitools#139)<#139 (comment)>
Thanks for looking into this @kevinmcody<https://github.com/kevinmcody> !
Looking into multipleOf I must say I misinterpreted this property somewhat. What I didn´t consider, for example, is that multipleOf: 0.7 on an integer type is valid; -7, 7, 14, etc. would be valid; equivalent to multipleOf: 7.
Considering the phrasing of the JSON specification, the annotation for multipleOf should be changed to multipleOf: float | None = Field(default=None, gt=0).
Using Decimal internally is a good idea (I should probably refactor the NumberSchema further) and considering multipleOf is a float for both IntegerSchmea and NumberSchema, the calculation logic can be extraced to a function that takes the minimum value, the maximum value and the multipleOf value and returns a tuple of k_min and k_max.
Note that annotation of the multipleOf as a gt=0 means checking that it's a positive number is no longer needed.
Also I don´t check for min > max within the models. This is a deliberate choice; if the schema does not make sense, the request data will simply fail schema validation without the need of extra validations / checks within the models / value generation.
The ToDo's for the get_values_out_of_bounds are also not quite straightforward; e.g. multipleOf = 1 for an IntegerSchema shouldn´t break get_values_out_of_bounds and the NumberSchema should include samples where precision is the issue. The test set for the JSON schema has some good examples:
https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/tests/draft2020-12/multipleOf.json#L67
Before this can be released, the tests in test_get_valid_value and test_get_values_out_of_bounds.py should also be updated to cover the new code in the oas_models.
I've created a branch to continue the work on this feature: https://github.com/MarketSquare/robotframework-openapitools/tree/76_add_support_for_multipleof
Can you target that branch with this PR / further changes? I'll see if I can get some of the tests added on that branch this Wednesday.
—
Reply to this email directly, view it on GitHub<#139 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANIDU2Q5DQV3NRW7F3LDULD4X5LPZAVCNFSM6AAAAACX72K3A6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGMRXGIYDEMBRG4>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
That's great. I might have some time this weekend to help out / work on this if you want to. I feel a TDD approach would be helpful here; get the tests in first (incl. the edge cases) so we get a clear idea on how the implementation should work, then get the implementation done. A contributions guide is still missing, but if you're working from the devcontainer setup, the |
|
I’ve never done unit tests in Python before, but I will look into that. What better time to learn?
While we work on this little side project, knowing that neither of us has a great deal of spare time to devote to it, would you mind JUST accepting the change to the multipleOf parameter (accepting an int, float, or null), just so we can have multipleOf in our schema without everything blowing up?
From: Robin ***@***.***>
Sent: Wednesday, April 29, 2026 10:01 AM
To: MarketSquare/robotframework-openapitools ***@***.***>
Cc: Kevin Cody ***@***.***>; Mention ***@***.***>
Subject: Re: [MarketSquare/robotframework-openapitools] Update oas_models.py to support multipleOf (PR #139)
[Image removed by sender.]robinmackaij left a comment (MarketSquare/robotframework-openapitools#139)<#139 (comment)>
Thanks for looking it over. I was also a little intimidated by the invalid values, that’s why I sheepishly left them as TODOs. 😊 I will certainly get on your suggested refactor, and look at the tests though, probably sometime this week.
That's great. I might have some time this weekend to help out / work on this if you want to. I feel a TDD approach would be helpful here; get the tests in first (incl. the edge cases) so we get a clear idea on how the implementation should work, then get the implementation done.
A contributions guide is still missing, but if you're working from the devcontainer setup, the inv utests is a useful command to quickly run the unit tests.
—
Reply to this email directly, view it on GitHub<#139 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANIDU2WIKG4SBB4TBPVTK4L4YIDKNAVCNFSM6AAAAACX72K3A6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGNBUGQZTKNBXGQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Fixes #76
Support multipleOf constraint on an Integer or Number.
Strategy: Use the ceiling of Min divided by multipleOf, floor of Max divided by multipleOf, for the random min/max, then multiply by multipleOf