Skip to content

NaN and infinity check for isNumber #5846

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

NaN and infinity check for isNumber #5846

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c436714
Added finite value check to isNumber
oscarczer Oct 23, 2022
2676f9d
Added a null checker in isNumber
oscarczer Oct 25, 2022
ad1ab55
Added unit tests
oscarczer Oct 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/core/friendly_errors/validate_params.js
View file
Original file line number Diff line number Diff line change
@@ -344,9 +344,9 @@ if (typeof IS_MINIFIED !== 'undefined') {
const isNumber = param => {
switch (typeof param) {
case 'number':
return true;
return isFinite(param);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think finite number check should be implemented here as mentioned in the original issue there could still be utility for using Infinity in certain circumstances and if drawing functions are ever to enforce finite only numbers they should be done in the drawing functions themselves instead of in the general case here.

case 'string':
return !isNaN(param);
return !(isNaN(param) || param == "null");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another special case that needs to be accounted for here which is when param === "" or param === " " for any number of spaces in the string. Accounting for this can potentially add some complexity, alternatively we can attempt to do parseFloat(param) first to directly test if the result is NaN which also have the benefit of possibly not needing to use different switch cases here.

default:
return false;
}
22 changes: 22 additions & 0 deletions test/unit/core/error_helpers.js
View file
Original file line number Diff line number Diff line change
@@ -177,6 +177,28 @@ suite('Error Helpers', function() {
'ValidationError type is correct'
);
});

testUnMinified('line: null string given', function() {
let err = assert.throws(function() {
p5._validateParameters('line', [1, 2, 4, "null"]);
}, p5.ValidationError);
assert.strictEqual(
err.type,
'WRONG_TYPE',
'ValidationError type is correct'
);
});

testUnMinified('line: infinite value given', function() {
let err = assert.throws(function() {
p5._validateParameters('line', [1, 2, 4, Infinity]);
}, p5.ValidationError);
assert.strictEqual(
err.type,
'WRONG_TYPE',
'ValidationError type is correct'
);
});
});

suite('validateParameters: trailing undefined arguments', function() {