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

maybe, either: remove S.maybeToEither and S.eitherToMaybe #664

Merged
merged 1 commit into from
Aug 9, 2020

Conversation

davidchambers
Copy link
Member

Before:

S.maybeToEither (x)
S.eitherToMaybe

After:

S.maybe (S.Left (x))
        (S.Right)
S.either (S.K (S.Nothing))
         (S.Just)

I appreciate the concern @Avaq expressed in #644 (comment):

My only concern is that in implementing the entire matrix of conversions between a, Maybe a, Either a b, and Either b a, we might be increasing API surface drastically for functions that can be expressed as simple compositions.

If others agree with this change, I would like to merge this pull request then rebase and merge #644. If you do not think these two functions should be removed, please say so. :)

/cc @futpib, @masaeedu

Comment on lines -4278 to +4335
return B (filter (pred)) (B (eitherToMaybe) (encase (JSON.parse)));
return B (filter (pred))
(B (either (K (Nothing)) (Just))
(encase (JSON.parse)));
Copy link
Member Author

Choose a reason for hiding this comment

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

I find the new implementation just as clear as the existing one. either (K (Nothing)) (Just) is the natural transformation from either to maybe. :)

@masaeedu
Copy link
Member

Things like this can probably go into a hypothetical sanctuary lens library, which would make it easier to convert back and forth and fill in cells in the "conversion matrix" via ad-hoc composition.

Copy link
Member

@Avaq Avaq left a comment

Choose a reason for hiding this comment

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

I think it's fine to remove these functions, as long as #644 is merged to provide alternatives.

@davidchambers davidchambers merged commit eba8d71 into master Aug 9, 2020
@davidchambers davidchambers deleted the davidchambers/maybe-either branch August 9, 2020 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants