revalidate icon indicating copy to clipboard operation
revalidate copied to clipboard

isNumeric – misleading name?

Open jamesplease opened this issue 8 years ago • 7 comments

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?

jamesplease avatar Apr 30 '17 06:04 jamesplease

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.

jfairbank avatar May 01 '17 21:05 jfairbank

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 :+1: with it continuing to support only strings
  • I'm :+1: 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 😛

jamesplease avatar May 01 '17 22:05 jamesplease

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.

jfairbank avatar May 02 '17 21:05 jfairbank

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.

jamesplease avatar May 02 '17 21:05 jamesplease

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.

jfairbank avatar May 02 '17 21:05 jfairbank

... Thoughts?

:+1: !

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

I'll hop to it.

Thanks @jfairbank !

jamesplease avatar May 02 '17 22:05 jamesplease

Hello

I need isNumber. Any news about this please ?

Bests

lauterry avatar May 16 '19 08:05 lauterry