Incorrect conversion of number to boolean #5209
Conversation
Check for min/max/stepSize coerces numbers to truthy values. A min setting of 0 is therefore interpreted as not being set. Checking against undefined means settings are correctly detected.
src/scales/scale.linearbase.js
Outdated
| @@ -25,7 +25,7 @@ function generateTicks(generationOptions, dataRange) { | |||
| var niceMax = Math.ceil(dataRange.max / spacing) * spacing; | |||
|
|
|||
| // If min, max and stepSize is set and they make an evenly spaced scale use it. | |||
| if (generationOptions.min && generationOptions.max && generationOptions.stepSize) { | |||
| if (generationOptions.min !== undefined && generationOptions.max !== undefined && generationOptions.stepSize !== undefined) { | |||
There was a problem hiding this comment.
is it wise to also check for NaN, infinite values, or values of other types. Perhaps if (!isNaN(generationOptions.min) && isFinite(generationOptions.min) ... )
There was a problem hiding this comment.
I feel that if we were going to check for those values we should probably log an error or throw an exception or somehow tell the user they've passed in invalid values rather than just silently ignoring the parameters. And maybe we should have a single method to validate user options called in the constructor or something rather than placing that code here
But in any case I feel it's quite unlikely for a user to set their options to Infinity, so I'm not sure I would require it before merging this PR. It would be a nice thing to do and I wouldn't object to validating user options, but this seems like a positive change even without that further improvement, so I don't want to block this PR on it necessarily
There was a problem hiding this comment.
At least we could check for undefined and null since setting null is quite common to ignore an option:
if (!helpers.isNullOrUndef(generationOptions.min) &&
!helpers.isNullOrUndef(generationOptions.max) &&
!helpers.isNullOrUndef(generationOptions.stepSize) {
// ...
}
|
Is there any issue associated to this PR? Don't we want a unit test to prevent regression? |
|
@teroman will you be able to update this PR to address the suggestions? |
|
I've edited the file in line with the suggestion to check for nulls. I have written a test in scale.linear.tests.js to check 0 is used as max setting. |
|
@etimberg @simonbrunel this is a high-quality PR with a test. Should be a good candidate for merging. Can you take a look? |
|
@etimberg @simonbrunel just a reminder that this PR is probably ready to merge |
|
Ideally, we should also test |
|
@teroman would you be able to update? thanks! |
src/scales/scale.linearbase.js
Outdated
| @@ -25,7 +25,7 @@ function generateTicks(generationOptions, dataRange) { | |||
| var niceMax = Math.ceil(dataRange.max / spacing) * spacing; | |||
|
|
|||
| // If min, max and stepSize is set and they make an evenly spaced scale use it. | |||
| if (generationOptions.min && generationOptions.max && generationOptions.stepSize) { | |||
| if (!helpers.isNullOrUndef(generationOptions.min) && !helpers.isNullOrUndef(generationOptions.max) && !helpers.isNullOrUndef(generationOptions.stepSize)) { | |||
There was a problem hiding this comment.
@teroman I just realized that we should not apply the same logic to stepSize because stepSize === 0 is invalid (we divide by stepSize line 30).
Should be:
if (!helpers.isNullOrUndef(generationOptions.min) && !helpers.isNullOrUndef(generationOptions.max) && generationOptions.stepSize) {Good spot, I must have been on autopilot!
When 0, the min and max options was considered not being set, instead we should check for null or undefined.
Check for min/max/stepSize coerces numbers to truthy values. A min setting of 0 is therefore interpreted as falsy, and not being set.
Checking against undefined means settings are correctly detected.