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

fix handling of NaN in Number$prototype$lte to guarantee totality #88

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

davidchambers
Copy link
Member

Currently, Z.lte(NaN, 0) and Z.lte(0, NaN) both evaluate false, violating the totality law.

I arbitrarily decided to make NaN the “smallest” Number value.

/cc @CrossEye

@Avaq
Copy link
Member

Avaq commented Feb 26, 2018

Why do we consider NaN to be a number, but null to be its own special "Null" type?

> typeof NaN
'number'
> typeof null
'object'

> type(NaN)
'Number'
> type(null)
'Null'

@Avaq
Copy link
Member

Avaq commented Feb 26, 2018

Ah I found the answer:

> Object.prototype.toString.call(NaN)
'[object Number]'
> Object.prototype.toString.call(null)
'[object Null]'

@Avaq
Copy link
Member

Avaq commented Feb 26, 2018

I'm looking for a reason to exclude NaN from having Number privileges.

@davidchambers
Copy link
Member Author

I'm looking for a reason to exclude NaN from having Number privileges.

I think this library should remain unopinionated, so should support all Number values. Libraries which depend on sanctuary-type-classes are free to be opinionated by excluding problematic values where appropriate. Sanctuary is certainly opinionated (it disallows Number objects, for example).

@Avaq
Copy link
Member

Avaq commented Feb 26, 2018

We might end up being forced to form an opinion either way, because of Symbol.toStringTag and how we rely on Object.prototype.toString to provide reliably information about built-ins.

@davidchambers
Copy link
Member Author

I'll give people a day or two to comment on this pull request before I merge it.

@davidchambers davidchambers merged commit f758e17 into master Feb 28, 2018
@davidchambers davidchambers deleted the davidchambers/lte branch February 28, 2018 22:31
@CrossEye
Copy link

CrossEye commented Mar 1, 2018

Well, now that it's committed, I finally manage to look at it.

But it certainly seems fine to me. Somehow you need to deal with this case, and the native behavior is incoherent. The decision is arbitrary, but at least it's coherent.

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 this pull request may close these issues.

3 participants