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

Adds toEither :: a -> b? -> Either a b #249

Merged
merged 1 commit into from
Sep 2, 2016

Conversation

rjmk
Copy link
Contributor

@rjmk rjmk commented Sep 2, 2016

A function for converting a possibly null-y value to an Either
Closes #83

@davidchambers
Copy link
Member

Very nice! I'll make several comments about minor issues, but they should all be easy to address. :)


it('returns Left of the first argument when second argument is `null`', function() {
eq(S.toEither('a', null), S.Left('a'));
});
Copy link
Member

Choose a reason for hiding this comment

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

Let's include an assertion for S.toEither('a', undefined).

@rjmk
Copy link
Contributor Author

rjmk commented Sep 2, 2016

@davidchambers Do you prefer that I add commits separately during the review process and squash at the end, or just push up the amended commit as I fix?

//.
//. Takes a failure value, 'x', and a possibly null or undefined value, 'y'.
//. Returns a Right of 'y' if it is defined and not null, otherwise returns
//. a Left of 'x'.
Copy link
Member

Choose a reason for hiding this comment

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

I find mention of "'x'" and "'y'" a bit confusing. I think we could get away with referring to "the first argument" and "the second argument".

How about this?

Converts an arbitrary value to an Either: a Left if the value is null or undefined; a Right otherwise. The first argument specifies the value of the Left in the "failure" case.

I'd like to improve the descriptions of many of the library's functions. In many cases the description is just an English translation of the type signature. In this case, for example, a direct translation would be:

Returns a Left of its first argument if its second argument is null or undefined; a Right of its second argument otherwise.

I like the mention of a "failure value" in your description. I'd like to avoid mention of "'x'" and "'y'" though.

Copy link
Contributor Author

@rjmk rjmk Sep 2, 2016

Choose a reason for hiding this comment

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

Agreed! I'd modelled this after encaseEither, but I guess naming arguments is more important when some are functions

@davidchambers
Copy link
Member

Do you prefer that I add commits separately during the review process and squash at the end, or just push up the amended commit as I fix?

I'm happy either way. Use whichever process works better for you.

@rjmk
Copy link
Contributor Author

rjmk commented Sep 2, 2016

@davidchambers I think that's everything addressed

@davidchambers
Copy link
Member

Thanks for the quick updates. I have three more requests:

  • Please use single-quoted strings in the examples (sorry, the linter doesn't catch this).
  • Let's drop the // from the protocol. location.protocol does not include the slashes, and the regexp literal will look less noisy without the \/\/.
  • toMaybe is defined between fromMaybe and maybe. Let's move the definition of toEither to between fromEither and either.

@rjmk
Copy link
Contributor Author

rjmk commented Sep 2, 2016

A function for converting a possibly null-y value to an Either
@@ -1265,7 +1265,7 @@

//# toMaybe :: a? -> Maybe a
//.
//. Takes a value and returns Nothing if the value is null or undefined;
//. Takes a value and returns Nothing if the value is `null` or `undefined`;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@davidchambers
Copy link
Member

Very nice patch, @rjmk. Thank you for tolerating my nitpicking. :)

🌳

@davidchambers davidchambers merged commit 567812f into sanctuary-js:master Sep 2, 2016
@mrtnbroder mrtnbroder mentioned this pull request Sep 29, 2016
4 tasks
@davidchambers
Copy link
Member

$ npm install sanctuary@0.12.0

$ node
> const S = require('sanctuary')
undefined
> S.toEither
toEither :: a -> b -> Either a b

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.

2 participants