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

[N4S] Rule isNegative | isPositive #401

Closed
ealush opened this issue Sep 21, 2020 · 10 comments Β· Fixed by #433
Closed

[N4S] Rule isNegative | isPositive #401

ealush opened this issue Sep 21, 2020 · 10 comments Β· Fixed by #433
Labels
good first issue Good for newcomers

Comments

@ealush
Copy link
Owner

ealush commented Sep 21, 2020

Add a new enforce rule:

enforce(-5).isNegative() // βœ…
enforce(5).isNegative() // 🚨
enforce(5).isPositive() // βœ…
enforce(-5).isPositive() // 🚨
@ealush ealush added the good first issue Good for newcomers label Sep 21, 2020
@sagivStekolshik
Copy link

what should happen when:

enforce(0).isNegative() // ❓

or

enforce(0).isPositive() // ❓

@ealush
Copy link
Owner Author

ealush commented Sep 26, 2020

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.

@sagivStekolshik
Copy link

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

@ganeshpatil0101
Copy link
Contributor

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
Copy link

zivkaziv commented Oct 7, 2020

Is this issue still open?

@ealush
Copy link
Owner Author

ealush commented Oct 10, 2020

@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 πŸ™‚

@ganeshpatil0101
Copy link
Contributor

ganeshpatil0101 commented Oct 12, 2020

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

@ganeshpatil0101
Copy link
Contributor

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

@ealush
Copy link
Owner Author

ealush commented Oct 13, 2020

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

@ganeshpatil0101
Copy link
Contributor

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

@ealush ealush linked a pull request Oct 14, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants