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

isNumeric – misleading name? #52

Open
jamesplease opened this issue Apr 30, 2017 · 7 comments
Open

isNumeric – misleading name? #52

jamesplease opened this issue Apr 30, 2017 · 7 comments

Comments

@jamesplease
Copy link

The built-in isNumeric validator has unexpected behavior, I think. I'd expect a validator named isNumeric to support any real number, i.e.; 5, 100.23 and maybe even "4,54", but at least the first two.

Given its current behavior, do you think the name isInteger is more suitable, because only integers pass?

@jfairbank
Copy link
Owner

This is mainly due to a design decision when I first created revalidate with redux-form in mind. Revalidate was intended to work on string values, so isNumeric works on string numbers. Someone else raised a similar concern in #20 too.

I'm open to changing that behavior if people are finding value using revalidate outside of string HTML form values, though. I think the best fix might be changing the behavior and bumping the major version. I've neglected getting a release out for #48, so this could be a good opportunity to implement it along with the major bump.

@jamesplease
Copy link
Author

jamesplease commented May 1, 2017

Ah, sorry – I should have been more specific. I'm alright with it working on string values, as I understand this library's goal to work with values of inputs. I'm using this with forms, too, so no qualms there.

The issue is that this statement:

so isNumeric works on string numbers.

is misleading I think. With the current behavior, I think that it more accurately works on string integers. String numbers would include "100.2", but that string currently fails validation with isNumeric.

tl;dr

  • I'm 👍 with it continuing to support only strings
  • I'm 👍 with it not having any behavior change, but would recommend it be renamed to isInteger
  • If we keep the current name, I'd recommend we update the behavior to support all real numbers (i.e.; numbers with decimal values)

On second thought, I guess the characters of the string are numeric, so from that perspective the name is fine. And if we support "1.22" would someone want to support, say "1/2", which is also a valid number string? Maybe this is a bad suggestion 😛

@jfairbank
Copy link
Owner

Ah, gotcha! Honestly, I only ever used it with integers, so I had some blindness there. I'm okay supporting real numbers, but I would still like to make it major bump. So the best bet would probably be to change it to isInteger and reintroduce isNumeric for integers and real numbers with decimals.

@jamesplease
Copy link
Author

but I would still like to make it major bump.

That makes sense to me.

So the best bet would probably be to change it to isInteger and reintroduce isNumeric for integers and real numbers with decimals.

Do you have any concern about someone wanting support for something like "1/2", or does that not bother you? Also, would we want to support commas as the decimal mark; i.e., "122,33"?


Now that I realize that it's referring to numeric characters within the string, it seems less unexpected to me. So I'd also be fine leaving it as-is. Whatever you think is best! If you're also OK with the current behavior, then feel free to close this issue out :) If you're leaning toward changing the behavior, then let me know and I can put together a PR for that.

@jfairbank
Copy link
Owner

Now that I realize that it's referring to numeric characters within the string, it seems less unexpected to me.

That was initially how I intended it.

Renaming it to isInteger, deprecating isNumeric, and adding isNumber or isReal for all numbers might be the best way to make things explicit. I'm okay supporting numbers in addition to strings too to make it more general purpose. Thoughts?

A PR would be awesome! I have some stale commits that I've been needing to make a release with, so your couple PRs for this and #51 would be some nice additions to that release.

@jamesplease
Copy link
Author

... Thoughts?

👍 !

so your couple PRs for this and #51 would be some nice additions to that release.

I'll hop to it.

Thanks @jfairbank !

@lauterry
Copy link

Hello

I need isNumber. Any news about this please ?

Bests

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

No branches or pull requests

3 participants