imbrn/v8n

Include string holding a number in examples

bakert opened this issue ยท 8 comments

Wonderful library, thank you.

I had a poke around but didn't find anything for this. A common use case for this library is probably validating user input that comes in the form of strings.

v8n().number().test('23')
v8n().integer().test('23')

are both rightly false. But what's the appropriate way to test if a string contains a number/integer? It feels wrong to add a pre-validation validation step where we attempt to convert to an int and handle as a validation failure if it doesn't. But perhaps that's the intended usage. Or maybe I just missed it.

Thanks!

imbrn commented

Thank you man for your question!

So, @bakert unfortunately, we don't have a good implementation for this kind of situation yet. Actually, I don't even know how would be a good implementation for that.

Today, I use to use something like this:

v8n()
  .number(false)
  .test(parseInt('123')); // true

The number rule has this flag where you can inform if it should consider NaN and infinite values as valid numbers. Defaults to true.

So, in this example, if parseInt returns NaN, the validation will return false, for example:

v8n()
  .number(false)
  .test(parseInt('hello')); // false

I don't know if it's a good solution for this problem, but that's something that will work.

Feel free to suggest other approaches or even ideas of how we could implement a good feature for this case. ๐Ÿ˜„

Ah ok. That's roughly where I ended up.

validDay = v8n().integer().between(1, 31).test(Number(day))

I didn't use parseInt because of parseInt("1jfkdsalfja") === 1 which is just wonky to me.

If I wanted to generate some kind of helpful error message here (I don't) I would have to do part of my validation "outside" v8n to know "hey they string you sent me is not the string of a number" and then go into the v8n stuff. So I think it would make sense to include.

I guess my suggestion is something like:

validDay = v8n().acceptString.integer().between(1, 31).test(day)

??

I don't like the name acceptString much. Maybe it's a param to integer?

imbrn commented

Yes, @bakert it looks like a good idea. We just need to pay close attention to the way v8n works with its modifiers and rules.

In your example, acceptString is a modifier that is being applied to the integer rule. But this makes sense for others rules, too? Is it going to be ok with mixed rules? ๐Ÿ˜ฌ

Modifiers cannot be exclusive for just a rule. If this only makes sense with integer, maybe it should be part of the integer parameters. Or, if it's something that is expected for the whole validation, maybe it should be in v8n constructor parameters. ๐Ÿค”

Anyway it's a tricky one, and we must be carefull. But solving tricky problems is really fun. ๐Ÿ˜†

I can believe that validDay = v8n().integer(stringOk=true).between(1, 31).test(day) is the best API for it. But of course both number and integer (and others?) will want to be able to do this. And for string this could be a noop. So maybe it does belong in the chain. I bow to your wisdom :D

A parameter for integer is probably easiest. The issue with JavaScript is that any added parameters make code less readable, as the name of the parameter is not included in the code (unlike Python for example). I don't like this idea, it adds clutter.

We currently have the number and integer rules. Instead of adding yet another parameter to integer we could add a rule for "string-numbers". Laravel, as an example, does not have a number rule but rather a numeric rule. This rule will check for any number or string that is a number. Ideally the number or numeric rule would do all the validation for numbers that integer does not do. This would include stringed numbers.

v8n().number().test(12); // true
v8n().number().test('12'); // true
v8n().number().test(12.01); // true
v8n().number().test('12.01'); // true

The parameter for Infinity and NaN could stay.

I like this a lot.

number already exists but numeric works.

I'd add:

    v8n().numeric().test('12hello'); // false

to the test suite just to be explicit that we are avoiding the horror that is parseInt's behavior :D

imbrn commented

Very good @sebastianbarfurth

Woo!