Skip to content

Conversation

Leomaradan
Copy link

NaN cannot be checked as equality. This commit add a test based on isNaN

nikolayg and others added 2 commits July 12, 2019 17:24
NaN cannot be checked as equality. This commit add a test based on isNaN
@coveralls
Copy link

coveralls commented Aug 15, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 1e9c938 on Leomaradan:master into 6296889 on mattphillips:master.

Copy link
Contributor

@anko anko left a comment

Choose a reason for hiding this comment

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

Just some thoughts.

});

describe('nested NaN', () => {
test('returns empty object when there is nested NaN value', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test description confused me.

More concrete wording:

Suggested change
test('returns empty object when there is nested NaN value', () => {
test('treats NaN -> NaN as unchanged', () => {

if (typeof lhs === "number" && typeof rhs === "number") {
if (isNaN(lhs) && isNaN(rhs)) return {}; // NaN is equal
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use Number.isNaN to check both Number type and NaN value simultaneously.

- if (typeof lhs === "number" && typeof rhs === "number") { - if (isNaN(lhs) && isNaN(rhs)) return {}; // NaN is equal - } + if (Number.isNan(lhs) && Number.isNan(rhs)) return {}; // NaN is equal
Copy link

Choose a reason for hiding this comment

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

@anko you meant to cap the last 'n' --> Number.isNaN

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants