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

Added Bitraverse for Ior (#2144) #2159

Merged
merged 2 commits into from
Jan 23, 2018
Merged

Conversation

V-Lamp
Copy link
Contributor

@V-Lamp V-Lamp commented Jan 23, 2018

Added Bitraverse[Ior], as discussed in #2144

For the testing part, I followed the pattern found in for testing Bitraverse[Either], within cats.tests.EitherSuite

This is my first PR, let me know if I need do something around this.
...and thanks for all the good work!

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution, this is great! Just one small request that we use explicit pattern matching instead of fold for performance reasons :)

implicit val catsBitraverseForIor: Bitraverse[Ior] = new Bitraverse[Ior] {

def bitraverse[G[_], A, B, C, D](fab: Ior[A, B])(f: A => G[C], g: B => G[D])(implicit G: Applicative[G]): G[Ior[C, D]] =
fab.fold(
Copy link
Member

Choose a reason for hiding this comment

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

Can we replace this fold with an explicit pattern match?

)

def bifoldLeft[A, B, C](fab: Ior[A, B], c: C)(f: (C, A) => C, g: (C, B) => C): C =
fab.fold(f(c, _), g(c, _), (a, b) => g(f(c, a), b))
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

fab.fold(f(c, _), g(c, _), (a, b) => g(f(c, a), b))

def bifoldRight[A, B, C](fab: Ior[A, B], c: Eval[C])(f: (A, Eval[C]) => Eval[C], g: (B, Eval[C]) => Eval[C]): Eval[C] =
fab.fold(f(_, c), g(_, c), (a, b) => g(b, f(a, c)))
Copy link
Member

Choose a reason for hiding this comment

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

and here.

@V-Lamp
Copy link
Contributor Author

V-Lamp commented Jan 23, 2018

Thanks @LukaJCB, done

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thank you @V-Lamp Looks great :)

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Thanks! on build green

@codecov-io
Copy link

codecov-io commented Jan 23, 2018

Codecov Report

Merging #2159 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2159      +/-   ##
==========================================
- Coverage   94.67%   94.64%   -0.03%     
==========================================
  Files         328      328              
  Lines        5538     5548      +10     
  Branches      199      213      +14     
==========================================
+ Hits         5243     5251       +8     
- Misses        295      297       +2
Impacted Files Coverage Δ
core/src/main/scala/cats/data/Ior.scala 98.48% <100%> (-1.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbac00d...f7ce53f. Read the comment docs.

@LukaJCB LukaJCB merged commit a296b8e into typelevel:master Jan 23, 2018
@kailuowang kailuowang modified the milestone: 1.1 Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants