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

Test for valid numbers #76

Closed
iangilman opened this issue Jul 25, 2018 · 10 comments
Closed

Test for valid numbers #76

iangilman opened this issue Jul 25, 2018 · 10 comments

Comments

@iangilman
Copy link

Unfortunately v8n().number().test(NaN) returns true, which while technically correct according to JavaScript is usually not what you want.

A few options:

  • Have number return false for NaN.
  • Have another not.nan() check.
  • Have a finite() check which returns false for NaN, Infinity, and -Infinity.
@imbrn
Copy link
Owner

imbrn commented Jul 25, 2018

I think we should go with a rule finite.

Do you want to make a PR @iangilman?

@sbarfurth
Copy link
Collaborator

I’m gonna say this should be 1.2.0 so 1.1.0 can finally go out.

@sbarfurth sbarfurth added this to the v1.2.0 milestone Jul 25, 2018
@imbrn
Copy link
Owner

imbrn commented Jul 25, 2018

It's already out man!

@sbarfurth
Copy link
Collaborator

I definitely missed too much in two days :D

@imbrn
Copy link
Owner

imbrn commented Jul 25, 2018

But you helped a lot though in this version. Thank you, man! 🎉🎉

@sbarfurth
Copy link
Collaborator

To be honest number() should really return false for NaN regardless of an additional rule. The same is true for Infinity. These are odd quirks of JavaScript in my opinion. I would actually prefer to see something like a parameter for number() where you can specify is these two should be included.

@imbrn
Copy link
Owner

imbrn commented Jul 26, 2018

It seems to be a valid argument for number() returning false instead of an additional rule.

@jgpacheco
Copy link
Contributor

Somebody working on this? I can take if a good first issue.

@imbrn
Copy link
Owner

imbrn commented Jul 26, 2018

Great! Go ahead @jgpacheco.

@iangilman
Copy link
Author

Wow, things move fast around here! @imbrn Thank you for the invite; I was going to say I probably can't get to it right away, so I'm glad @jgpacheco has jumped in! :)

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

No branches or pull requests

4 participants