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 toIorT On EitherT #4108

Merged
merged 4 commits into from
Jan 12, 2022
Merged

Add toIorT On EitherT #4108

merged 4 commits into from
Jan 12, 2022

Conversation

isomarcte
Copy link
Member

Similar to toEither on IorT.

Similar to `toEither` on `IorT`.
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

👍 conceptually, but I don't think it's going to compile.

core/src/main/scala/cats/data/EitherT.scala Outdated Show resolved Hide resolved
isomarcte and others added 2 commits January 11, 2022 13:25
rossabaker
rossabaker previously approved these changes Jan 11, 2022
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

👍 if this one's green

@satorg
Copy link
Contributor

satorg commented Jan 11, 2022

I'd suggest adding a test for this new method into EitherTSuite.


/** Convert this `EitherT[F, A, B]` into an `IorT[F, A, B]`.
*/
def toIorT(implicit F: Functor[F]): IorT[F, A, B] =
Copy link
Contributor

@satorg satorg Jan 11, 2022

Choose a reason for hiding this comment

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

I am not sure about the name. For example, a conversion to OptionT has a name just toOption (no T at the end). Should we follow the same convention here and name it as just toIor instead?

Copy link
Member Author

@isomarcte isomarcte Jan 11, 2022

Choose a reason for hiding this comment

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

@satorg hmm. I had noticed something similar on IorT with toEither: EitherT[F, A, B]. I had initially assumed this was a typo, but maybe it is an intentional convention. It seems strange to me, as there are two distinct ways view an EitherT[F, A, B] as an optional value, OptionT[F, B] and F[Option[B]] and I could see having both toOptionT and toOptionF. It seems we are a bit inconsistent right now.

For example, in IorT.scala we have fromEither, which operates on Either[A, B] as opposed to EitherT[F, A, B] and fromEitherF which operates on F[Either[A, B]].

It seems like if we want to standardize on a convention which can work with A, F[A], and G[F, A] (where G is some transformer type), then we have to go with the F and T suffixes.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@isomarcte I'm not that certain about the reasoning behind that convention, but I guess that it just does not make a lot of sense to have both toOptionT returning OptionT and toOptionF returning F[Option[...]], because OptionT implies F[Option[...]] already via its value field. And seems the same is applicable for EitherT and IorT.

Copy link
Member Author

Choose a reason for hiding this comment

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

@satorg I'll update it. It does seem to be the convention.

Copy link
Member Author

Choose a reason for hiding this comment

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

@satorg updated.

@isomarcte
Copy link
Member Author

@satorg test added

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Good catch, @satorg. I don't like the convention, but it's good to follow it.

@satorg satorg merged commit 7876696 into typelevel:main Jan 12, 2022
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.

3 participants