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.

Sign up for GitHub

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

Jump to bottom

Should OptionT.mapFilter be implemented in terms of OptionT.subflatMap #2757

Closed
morgen-peschke opened this issue Mar 25, 2019 · 1 comment · Fixed by #2765
Closed

Should OptionT.mapFilter be implemented in terms of OptionT.subflatMap #2757

morgen-peschke opened this issue Mar 25, 2019 · 1 comment · Fixed by #2765

Comments

@morgen-peschke
Copy link
Contributor

Is there a use case for having different implementations of OptionT.mapFilter and OptionT.subflatMap?

There's already a test enforcing the invariant that mapFilter should be consistent with subflatMap. Given this, should mapFilter delegate to subflatMap? If not, I'd be happy to add the reason to the scaladocs.

Having two implementations doing the same thing is a bit confusing, particularly as EitherT.mapFilter does not exist.
@kailuowang
Copy link
Contributor

They came from two different contexts. mapfilter is a method in the FunctorFilter type class while subflatmap is more native to the data type.
I agree that having OptionT.mapFilter be implemented in terms of OptionT.subflatMap would be less confusing.

morgen-peschke added a commit to morgen-peschke/cats that referenced this issue Mar 28, 2019
@morgen-peschke
Define OptionT.mapFilter in terms of OptionT.subflatMap
473691d 
typelevel#2757
morgen-peschke added a commit to morgen-peschke/cats that referenced this issue Mar 28, 2019
@morgen-peschke
Define OptionT.mapFilter in terms of OptionT.subflatMap
3f19495 
fixes typelevel#2757
@morgen-peschke morgen-peschke mentioned this issue Mar 28, 2019
Merged
kailuowang pushed a commit that referenced this issue Mar 28, 2019
@morgen-peschke @kailuowang
Define OptionT.mapFilter in terms of OptionT.subflatMap (#2765)
9479ce5 
fixes #2757
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Define OptionT.mapFilter in terms of OptionT.subflatMap morgen-peschke/cats
2 participants
@kailuowang @morgen-peschke