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

SemigroupK consistency and fix; added compose tests. #751

Merged
merged 1 commit into from
Dec 16, 2015

Conversation

milessabin
Copy link
Member

  • Renamed SemigroupK composedWith to compose for consistency with
    Applicative, Foldable, Reducible.
  • Fixed non-termination bug in SemigroupK compose(dWith).
  • Added tests for Applicative, Foldable, Reducible, SemigroupK
    composition.

+ Renamed SemigroupK composedWith to compose for consistency with
  Applicative, Foldable, Reducible.
+ Fixed non-termination bug in SemigroupK compose(dWith).
+ Added tests for Applicative, Foldable, Reducible, SemigroupK
  composition.
@@ -31,9 +31,9 @@ import simulacrum.{op, typeclass}
/**
* Compose two SemigroupK intsances.
*/
def composedWith[G[_]: SemigroupK]: SemigroupK[λ[α => F[G[α]]]] =
def compose[G[_]: SemigroupK]: SemigroupK[λ[α => F[G[α]]]] =
Copy link
Contributor

Choose a reason for hiding this comment

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

We have Alternative which extends both Applicative and MonoidK. Not that calling it composedWith is a particularly elegant solution, but do we run the risk of overloading ambiguities/annoyances with both composes being on the same instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the right thing to do here is to add a compose to Alternative which composes a pair of Alternatives to an Alternative. I can add that to this PR or follow up afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a good solution to me. It might also mean we want to add something similar to MonoidK. I'm okay with that either going into this PR or a follow up one.

@codecov-io
Copy link

Current coverage is 87.61%

Merging #751 into master will increase coverage by +0.39% as of a144efe

@@            master    #751   diff @@
======================================
  Files          166     166       
  Stmts         2293    2293       
  Branches        74      74       
  Methods          0       0       
======================================
+ Hit           2000    2009     +9
  Partial          0       0       
+ Missed         293     284     -9

Review entire Coverage Diff as of a144efe

Powered by Codecov. Updated on successful CI builds.

new SemigroupK[λ[α => F[G[α]]]] {
def combine[A](x: F[G[A]], y: F[G[A]]): F[G[A]] = combine(x, y)
def combine[A](x: F[G[A]], y: F[G[A]]): F[G[A]] = self.combine(x, y)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@ceedubs
Copy link
Contributor

ceedubs commented Dec 15, 2015

👍

As mentioned here, there are probably some additional compose methods that should be added, but I'm okay with those either being added here or in a separate PR.

@milessabin
Copy link
Member Author

I'll do a couple of follow up PRs for the issues that @ceedubs raised.

@non
Copy link
Contributor

non commented Dec 16, 2015

I agree with @ceedubs' comments, and also his 👍

Thanks!

non added a commit that referenced this pull request Dec 16, 2015
SemigroupK consistency and fix; added compose tests.
@non non merged commit 77ca413 into typelevel:master Dec 16, 2015
@non non removed the in progress label Dec 16, 2015
@ceedubs
Copy link
Contributor

ceedubs commented Dec 16, 2015

Thanks, @milessabin!

@stew stew mentioned this pull request Jan 30, 2016
adelbertc added a commit that referenced this pull request Feb 1, 2016
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.

4 participants