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

CatsSemigroupKForTask added #706

Merged
merged 2 commits into from Sep 4, 2018

Conversation

Projects
None yet
3 participants
@jendakol
Contributor

jendakol commented Sep 4, 2018

Solves issue #703

class CatsSemigroupKForTask extends SemigroupK[Task] {
def combineK[A](a: Task[A], b: Task[A]): Task[A] =
Task.catsAsync.handleErrorWith(a)(_ => b)

This comment has been minimized.

@Avasil

Avasil Sep 4, 2018

Collaborator

I think we could use a.onErrorHandleWith(_ => b) directly but I'm not sure if it makes difference though

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Sep 4, 2018

Thanks for PR @jendakol -- looks good!

But could you add test in TypeClassLawsForTaskSuite to make sure it is and remains lawful instance? :)

@codecov

This comment has been minimized.

codecov bot commented Sep 4, 2018

Codecov Report

Merging #706 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #706      +/-   ##
==========================================
- Coverage   90.58%   90.56%   -0.02%     
==========================================
  Files         391      391              
  Lines       10991    10993       +2     
  Branches     2052     2064      +12     
==========================================
  Hits         9956     9956              
- Misses       1035     1037       +2
@alexandru

This comment has been minimized.

Member

alexandru commented Sep 4, 2018

Hey, nice.

  1. wouldn't this instance make better sense if integrated in CatsBaseForTask? https://github.com/monix/monix/blob/master/monix-eval/shared/src/main/scala/monix/eval/instances/CatsBaseForTask.scala
  2. if you do it for Task, we need it for Coeval as well
@alexandru

This comment has been minimized.

Member

alexandru commented Sep 4, 2018

Also, what @Avasil said, we need to test the laws, so checkout the TypeClassLaws* tests.

@jendakol

This comment has been minimized.

Contributor

jendakol commented Sep 4, 2018

@alexandru

  1. I don't know, you tell me 😃
  2. OK, np, but where to? object Coeval or somewhere else?

Sure, tests will be added.

@alexandru

This comment has been minimized.

Member

alexandru commented Sep 4, 2018

@jendakol yes, include the instance in CatsBaseForTask for Task and in CatsSyncForCoeval for Coeval.

@jendakol

This comment has been minimized.

Contributor

jendakol commented Sep 4, 2018

Updated. Implemented for both Task and Coeval, enabled laws tests.

@Avasil

Avasil approved these changes Sep 4, 2018

@alexandru alexandru merged commit 3ba16e2 into monix:master Sep 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment