[N4S] Rule isNegative | isPositive about vest HOT 10 CLOSED

@zivkaziv @ealush I can see other unit tests are failing. does that expected ? I will add unit test for isNegative & isPositive and then raise pr.

from vest.

@ealush @zivkaziv unit tests are working fine. But on my local example of vest with vanilla is not working. I will check it. Thanks.

from vest.

what should happen when:

enforce(0).isNegative() // ❓

or

enforce(0).isPositive() // ❓

from vest.

Hey @sagivStekolshik, that's an excellent question.
The truth is that whether zero is positive or negative is context dependent, and sometimes it is regarded as neither.

This means that we can arbitrarily decide how we want to treat it.

For example, we could decide that in both cases it is false

enforce(0).isNegative() // false
enforce(0).isPositive() // false

or even treat it always as true

enforce(0).isNegative() // true
enforce(0).isPositive() // true

In our case, I think it would be most intuitive to start with the negative case:

negative is everything that's smaller than zero value < 0

So anything else would be considered as positive.
In that scenario 0 would be treated as positive.

The one exception is - how to deal with signed zero -0, because:

-0 === 0 // true
Object.is(-0, 0) // false

But since signed zero is so rare in js, it might not even be worth looking at.

from vest.

My idea was using Math.sign() to get the number or string(works as well)
sign function returns 0 or -0 according to MDN
so going the 0 -> false route will be easier
going the 0 isPositive number will be even easier as it can be the "negative" of isNegative

from vest.

Hi @ealush
I would like to work on this issue.
Please assign to me. I already started working on it and is there any diagram or documentation to get started?
Thanks,

from vest.

Is this issue still open?

from vest.

@ganeshpatil0101

Hi @ealush
I would like to work on this issue.
Please assign to me. I already started working on it and is there any diagram or documentation to get started?
Thanks,

@zivkaziv

Is this issue still open?

Yes. No need to assign anyone, simply submit a pr and we'll get it merged 🙂

from vest.

@ganeshpatil0101 do you have a branch you can share with those changes that do not work? I might be able to help.

from vest.

@ealush Please review and merge the PR #433
New unit tests are added & all tests are passed.
I request you to test this feature with one real example as I am having issues to setup a vest - vanilla example on local.
Please let me know you if there any changes required.
Thanks.

from vest.