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

Add EitherMaybe conversion functions #644

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

futpib
Copy link
Contributor

@futpib futpib commented Jul 31, 2019

This pull request adds the following functions:

maybeToLeft  :: b -> Maybe a -> Either a b
maybeToRight :: a -> Maybe b -> Either a b

leftToMaybe  :: Either a b -> Maybe a
rightToMaybe :: Either a b -> Maybe b

//. Right ('No negative numbers')
//.
//. > S.maybeToLeft ('No negative numbers') (S.find (S.lt (0)) ([-1, 0, 1]))
//. Left (-1)
Copy link
Member

Choose a reason for hiding this comment

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

These are nice examples. :)

index.js Outdated
}
_.fromEither = {
consts: {},
types: [$.Either (a) (a), a],
impl: fromEither
Copy link
Member

Choose a reason for hiding this comment

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

Could we use either (I) (I) here directly?

index.js Outdated
_.fromLeft = {
consts: {},
types: [a, $.Either (a) (b), a],
impl: fromLeft
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of using B (either (I)) (K) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a difference, except perhaps stylistic, so you decide.

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 stick with your implementation. :)

index.js Outdated
consts: {},
types: [b, $.Either (a) (b), b],
impl: fromEither
impl: fromRight
Copy link
Contributor Author

@futpib futpib Jul 31, 2019

Choose a reason for hiding this comment

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

@davidchambers Should use B (C (either) (I)) (K) here to match fromLeft?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 the existing implementation of fromRight easy to read. Let's keep it. Let's also keep your implementation of fromLeft, as there is value in similar things looking similar. :)

@davidchambers
Copy link
Member

What do others think of these (breaking) changes? @Avaq? @Bradcomp? @masaeedu?

@masaeedu
Copy link
Member

masaeedu commented Aug 1, 2019

@davidchambers do we have a swapEither :: Either a b -> Either b a function? This sort of thing might be better handled by a lens library, but absent a good story for those, functions like the one in this PR can be pretty handy.

@davidchambers
Copy link
Member

We have swap :: Pair a b -> Pair b a but not swapEither :: Either a b -> Either b a. Could you explain the relationship you see between swapEither, a lens library, and the functions in this pull request? You seem to be suggesting that there are multiple ways to express these ideas, and that a different approach could render these changes unnecessary.

@masaeedu
Copy link
Member

masaeedu commented Aug 1, 2019

@davidchambers Right, there's a number of different alternatives here. FWIW I think going down the road of exhaustively comparing all these alternatives would swamp the PR, and I think the functions in the PR are simple and handy enough to justify addition as is.

To explain the relationship between the functions here and swapEither: if I understand the changes here correctly, they're implementing a "swapped" version of each xyzEither function.

So for example instead of only having maybeToEither :: a -> Maybe b -> Either a b, it adds maybeToLeft :: a -> Maybe b -> Either b a, which could also be seen as a composition of maybeToEither and swapEither.

Similarly, instead of only having fromEither :: b -> Either a b -> b, you additionally have fromLeft :: b -> Either b a -> b, which again can be seen as b => compose(fromEither(b))(swapEither).

There's also wacky stuff you can do with lenses (have an iso between Either a b and Either b a, and compose it with optics relating maybes and eithers), but there's no good story for this at the moment, and it'd be a ways off.

@davidchambers
Copy link
Member

Thanks for the clarification, Asad. I understand your point now. :)

@Avaq
Copy link
Member

Avaq commented Aug 5, 2019

What do others think of these (breaking) changes?

I'm happy with them. 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. The current "matrix":

from ⬇️ \ to ➡️ a Maybe a Either a b Either b a
a I Just Left Right
Maybe a fromMaybe I maybeToLeft maybeToRight
Either a b fromLeft leftToMaybe I swapEither
Either b a fromRight rightToMaybe swapEither I

Instead of leftToMaybe, one could use a composition of Just and fromLeft, for example. Or the other way around, use a composition of leftToMaybe and fromMaybe to derive fromLeft.

My concern exists because the composition model is easily scalable under inclusion of additional types, whereas the above matrix would grow exponentially with every new constructor added.


I've just spent a lot of words explaining this concern, which makes it seem as though I might feel strongly about this, but it's really very minor - just something you might not have considered.

@Avaq
Copy link
Member

Avaq commented Sep 28, 2019

Going over these changes again, I think they represent an improvement over the status quo. My API surface scaling concern is small, because these functions will always have descriptive names / namespaces.

@davidchambers
Copy link
Member

@futpib, are you interested in submitting a small pull request to change S.fromEither as you did here?

@futpib
Copy link
Contributor Author

futpib commented Aug 9, 2020

I also dropped the commit with new fromEither from this PR. This is not a major change now, but #690 is.

@davidchambers
Copy link
Member

Lovely work, @futpib. Thank you for pointing out that this pull request no longer contains breaking changes.

Please update the commit messages and this pull request's title to reflect the fact that this pull request is purely additive now that #664 has been merged. Also, this pull request's description is no longer accurate.

@futpib futpib changed the title Split Either functions to Left and Right pairs Add EitherMaybe conversion functions Aug 11, 2020
@futpib
Copy link
Contributor Author

futpib commented Aug 11, 2020

@davidchambers Done.

@davidchambers
Copy link
Member

Thank you, @futpib. This pull request is in a very good state. :)

I am not certain that these functions should be added to the library. Here they are in use:

> maybeToRight ('XXX') (Nothing)
Left ("XXX")

> maybeToRight ('XXX') (Just (42))
Right (42)

> rightToMaybe (Left ('XXX'))
Nothing

> rightToMaybe (Right (42))
Just (42)

The transformations above are possible with existing functions:

> maybe (Left ('XXX')) (Right) (Nothing)
Left ("XXX")

> maybe (Left ('XXX')) (Right) (Just (42))
Right (42)

> either (K (Nothing)) (Just) (Left ('XXX'))
Nothing

> either (K (Nothing)) (Just) (Right (42))
Just (42)

To me, the expressions involving maybe and either are at least as clear as those involving the specialized functions.

Note that maybeToRight can return a Left, which makes sense but reveals the trouble with naming things (see also NaN).

Do these functions offer enough value to justify their inclusion? I would like to hear from @masaeedu, @Avaq, and others. :)

@Avaq
Copy link
Member

Avaq commented Aug 11, 2020

Note that maybeToRight can return a Left, which makes sense but reveals the trouble with naming things (see also NaN).

Should we consider justToRight?

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.

None yet

4 participants