-
Notifications
You must be signed in to change notification settings - Fork 59
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
Unexpected behavior for step rounding according to spec in Integer/Float ranges when min
or max
are missing
#301
Comments
Nice catch. The 2nd example is mathematically correct, but counter intuitive imho. So we should specify how it should round in order to result in the intuitive value. |
base should be calculated like this I think:
|
Wondering; should we be creating a generic test suite for all sorts of validations? That would ease writing homie libraries quite a bit I think
similar to these json schema ones: https://github.com/json-schema-org/JSON-Schema-Test-Suite/tree/main/tests/draft4 |
I actually stumbled across this while writing extensive tests for my rust library. |
sorry should have been more clear. Intent is not to use JSON schema for validation, but writing test cases in JSON, like this one: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/tests/draft4/anyOf.json it is a file that lists all the test cases, and expected results for the JSONschema "anyOf" operator. So instead of defining all the testcases yourself, you parse the JSON file, and in a loop run all the tests contained within it. Example: This module; https://github.com/Tieske/lua-resty-ljsonschema/tree/master/spec contains the repo above as a submodule. In the test suite tyhe files are loaded, and tests executed: https://github.com/Tieske/lua-resty-ljsonschema/blob/master/spec/suite_spec.lua |
we could create one centralized set of tests, here in the homiet github org, and every library can run tests against those sets. That would cover your comment above:
|
Ah now I get it. Yes I think that is a great idea! |
first attempt here: https://github.com/homieiot/homie-testsuite/blob/main/homie5/formats/boolean.yml please check, then we can add the numeric stuff as well PS. I used yaml instead of json, because it supports comments |
I would prefer having the defining properties of the test under one attribute so we can directly parse it in as the object to be tested where applicable: tests:
- description: proper format is accepted
property:
type: boolean
format: 'off,on'
valid: true This way I could parse the property description directly into the type used in my library also using the same mechanism as I do for the real thing instead of manually constructing a property description. To make it more generic we could change it to: tests:
- description: proper format is accepted
test_definition:
type: boolean
format: 'off,on'
input_data: {}
valid: true naming is as always hard, so test_definition and input _data we're just the first thing that came to mind. |
that's the part I struggled with. Changing it like that is fine by me |
Let me try actually implementing this for my library before we do more definitions to see if it works out as expected. I should have some time on Sunday night. |
@Tieske : I created a pull request to your test repo. Please have a look and let me know what you think. |
The main issue is that when calculating the steps before rounding, we take the difference and then divide by step-size. If To prevent that, the base should be "stepped down" to be smaller than the value being rounded. To take the math out of the above, it could also be done like this, using a loop, does the same thing but probably easier to grasp;
|
Description:
The current spec for handling integer/float ranges with steps,
[min]:[max][:step]
, specifies that the base for calculating a proper value based on the step should bemin
,max
, or the current property value (in that order). However, the behavior when eithermin
ormax
is missing results in different rounding behavior that is not explicitly clarified in the spec.Specifically, the behavior of rounding changes depending on whether the base for step rounding is derived from the
min
ormax
value. This leads to different rounding results in cases such as0:10:2
vs:10:2
, where:0:10:2
, the base ismin
(0), and the rounding behaves as expected, rounding values upwards from0
.:10:2
(no lower bound), the base ismax
(10), which results in rounding values downwards from10
.This results in inconsistencies that may not be immediately obvious from reading the spec.
Examples:
Example 1: Range
0:10:2
0
, a maximum of10
, and a step of2
. The base for rounding ismin
(0), so rounding occurs upwards from the base.Example 2: Range
:10:2
10
, and a step of2
. The base for rounding ismax
(10), which causes rounding to occur downwards from the maximum value.max
)max
)max
)Issue:
The spec does not clearly indicate the expected behavior when rounding from
max
vs.min
. While the spec indicates that the base for step calculation should be eithermin
,max
, or the current property value (in that order), it is not clear that the rounding behavior would differ significantly depending on which base is used. This could lead to confusion for implementers who expect consistent rounding behavior regardless of whethermin
ormax
is specified.Request:
We should improve the spec by:
min
ormax
is used as the base for step calculation.min
is missing (i.e., rounding down frommax
).This clarification would help ensure consistent understanding of how the spec should be implemented and what behavior is expected.
Or think about if we want to adjust the spec to ensure the rounding results stay consistent in both cases.
The text was updated successfully, but these errors were encountered: