Skip to content
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

Allow sorting NaN values #2

Closed
Richienb opened this issue May 23, 2020 · 15 comments · Fixed by #3
Closed

Allow sorting NaN values #2

Richienb opened this issue May 23, 2020 · 15 comments · Fixed by #3

Comments

@Richienb
Copy link
Contributor

Richienb commented May 23, 2020

When sorting NaN values, they should be treated as the lowest value in the entire array:

const numberSort = require('num-sort');

console.log([1, NaN, 4, 3].sort(numberSort.ascending));
//=> [NaN, 1, 3, 4]

cc @sindresorhus

@papb
Copy link
Contributor

papb commented May 23, 2020

What is the current behavior?

@Richienb
Copy link
Contributor Author

It currently throws TypeError: Expected a number.

@papb
Copy link
Contributor

papb commented May 23, 2020

What is the reasoning for it being considered lowest rather than highest?

@Richienb
Copy link
Contributor Author

NaN means not a number which means nothing which means 0.

@papb
Copy link
Contributor

papb commented May 23, 2020

Well but 0 is not the lowest possible number 😅

@Richienb
Copy link
Contributor Author

0 is the smallest natural number and is usually a fallback for NaN so it's obvious for it to be the smallest number.

@papb
Copy link
Contributor

papb commented May 23, 2020

What do you think should be the expected result for [1, -1, NaN].sort(numberSort.ascending)?

@Richienb
Copy link
Contributor Author

[NaN, -1, 1]

@papb
Copy link
Contributor

papb commented May 23, 2020

So you're treating NaN as -Infinity, not 0.

My opinion is that this is arbitrary and often not intended. How about, instead, adding an option?

const numberSort = require('num-sort');

console.log([1, NaN, -4, 3].sort(numberSort.ascending({ treatNanAs: -Infinity })));
//=> [NaN, -4, 1, 3]

@sindresorhus
Copy link
Owner

I agree, NaN should be last, after -Infinity.

@sindresorhus
Copy link
Owner

So you're treating NaN as -Infinity, not 0.

Infinity is technically a number. NaN is not, hence it should be first/last.

@papb
Copy link
Contributor

papb commented May 23, 2020

@sindresorhus Do you prefer a breaking change (simply change the existing behavior) by arbitrarily choosing between first/last, or do you think an option such as I suggested is possible?

@sindresorhus
Copy link
Owner

I prefer a breaking change.

@papb
Copy link
Contributor

papb commented May 23, 2020

@sindresorhus One last confirmation:

[1, -1, -Infinity, NaN].sort(numberSort.ascending)
//=> [NaN, -Infinity, -1, 1]

[1, -1, -Infinity, NaN].sort(numberSort.descending)
//=> [1, -1, -Infinity, NaN]

Is this correct? If yes I'll submit a PR

@sindresorhus
Copy link
Owner

Correct

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

Successfully merging a pull request may close this issue.

3 participants