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

isJust/isNothing without instanceof check #88

Merged

Conversation

stoeffel
Copy link
Contributor

@stoeffel stoeffel commented Jan 1, 2016

  • throwing an exception if not a Maybe is passed

refs #86

@@ -11,9 +11,15 @@ function _Just(x) {
}
util.extend(_Just, Maybe);

_Just.prototype.isJust = R.T;
_Just.prototype.isNothing = R.F;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not the following?

_Just.prototype.isJust = true;
_Just.prototype.isNothing = false;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would work as well. But I think it's more intuitive when isJust, isNothing, isLeft and isRight are functions.
But I could live with both.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer them both to have static properties on their prototypes rather than functions too (apologies for my earlier suggestion in #86 for using R.T/R.F). We still have the functions available on Maybe/Either for people to use where a function is more appropriate.

@stoeffel
Copy link
Contributor Author

stoeffel commented Jan 2, 2016

I changed isJust and isNothing on the instance to be a boolean. 🏁

@@ -29,11 +35,19 @@ Maybe.of = Maybe.Just;
Maybe.prototype.of = Maybe.Just;

Maybe.isJust = function(x) {
return x instanceof _Just;
if (typeof x.isJust !== 'boolean') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the typeof test is no longer necessary here (or in isNothing) and we can just return x.isJust === true;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we need to change the signature to Maybe.isJust :: Any -> boolean

see #86 your comment:

I wasn't initially going to suggest the use of typeof x.isLeft === 'function', but if Maybe.isJust is to be implemented the same way then it would also change from Any -> Boolean to Maybe a -> Boolean due to its current use of instanceof. I'd prefer Maybe a -> Boolean, but it would be a potentially breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still keep the types signature to specify that only Maybe instances are accepted. It's just a matter of whether we throw in the implementation if something other than a Maybe instance is given.

If we're to add type checking to functions, we will need to approach it more holistically such as the suggestion in #89.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convinced. will drop it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind doing the same in the Either PR #86? Then I think we're in a good position for both of them. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and done.

- throwing an exception if not a Maybe is passed
@stoeffel stoeffel force-pushed the is-maybe-without-instance-check branch from 0f440d1 to 5109673 Compare January 2, 2016 09:37
@scott-christopher
Copy link
Member

Thanks again, @stoeffel 🍄

scott-christopher pushed a commit that referenced this pull request Jan 2, 2016
isJust/isNothing without instanceof check
@scott-christopher scott-christopher merged commit 9b748c2 into ramda:master Jan 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants