jfairbank / revalidate

Elegant and composable validations
http://revalidate.jeremyfairbank.com
MIT License
363 stars 20 forks source link

isNumeric – misleading name? #52

Open jamesplease opened 7 years ago

jamesplease commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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


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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

... 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 !

lauterry commented 5 years ago

Hello

I need isNumber. Any news about this please ?

Bests