Feat: Add step property for number rule - #362
Conversation
icebob-ai
left a comment
There was a problem hiding this comment.
Thanks for the well-structured PR — all the pieces are there (rule, messages, types, tests, README). Nice work!
However, there's a floating point precision bug with the modulo approach:
0.3 % 0.1 // → 0.09999999999999998 (not 0!)
1.2 % 0.4 // → 0.3999999999999999So { type: "number", step: 0.1 } would incorrectly reject 0.3. The current tests only use integer steps (step: 10), which hides this issue.
Please fix:
- Handle floating point precision — e.g. using a tolerance check like
Math.abs(Math.round(value / step) - value / step) > 1e-10or a similar approach - Add test cases with float steps (e.g.
step: 0.1with values like0.3,0.7,1.2) - Consider validating that
stepis a positive number (what shouldstep: -5orstep: 0do?)
Small effort, but it'll make this feature solid. Thanks!
|
Thanks for the feedback! I've implemented the suggested tolerance check to handle floating-point precision issues. Regarding the I also updated the test suite to cover floating-point scenarios and invalid schema configurations. |
|
Previous tolerance-based approach allowed certain invalid values to pass, which was incorrect behavior for the validator. Current implementation eliminates false positives for floating-point numbers exceeding the old |
icebob-ai
left a comment
There was a problem hiding this comment.
Looks great now! The floating point handling with the integer multiplication approach is solid, schema validation catches invalid steps at compile time, and the test coverage is thorough (including float edge cases).
Minor nit: there are a few unrelated whitespace changes (double-space → single-space in makeError calls) — ideally those would be in a separate commit, but it's not a blocker.
Thanks for the quick turnaround and the quality work — merging! 🎉
Implementing
stepproperty maintains consistency with the HTML5 range and number input types, which utilize the step attribute.