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

Add Bifoldable, fixes #94 #864

Merged
merged 4 commits into from
Feb 17, 2016
Merged

Add Bifoldable, fixes #94 #864

merged 4 commits into from
Feb 17, 2016

Conversation

adelbertc
Copy link
Contributor

Also:

  • Add MonadCombine separate
  • Add serializability check for Bifunctor[Xor]

@adelbertc
Add Bifoldable, fixes typelevel#94
e0f1c3c 
Also:
- Add MonadCombine separate
- Add serializability check for Bifunctor[Xor]
@adelbertc
Copy link
Contributor Author

Hmm Travis isn't running - closing and re-opening to see what happens.

@adelbertc adelbertc closed this Feb 6, 2016
@adelbertc adelbertc reopened this Feb 6, 2016
@codecov-io
Copy link

Current coverage is 89.01%

Merging #864 into master will decrease coverage by -0.33% as of 1302795

@@            master    #864   diff @@
======================================
  Files          169     174     +5
  Stmts         2337    2384    +47
  Branches        75      75       
  Methods          0       0       
======================================
+ Hit           2088    2122    +34
  Partial          0       0       
- Missed         249     262    +13

Review entire Coverage Diff as of 1302795

Powered by Codecov. Updated on successful CI builds.

@adelbertc
Copy link
Contributor Author

There we go

ceedubs
ceedubs reviewed Feb 6, 2016
View reviewed changes
core/src/main/scala/cats/data/Xor.scala
@@ -166,9 +166,19 @@ private[data] sealed abstract class XorInstances extends XorInstances1 {
def combine(x: A Xor B, y: A Xor B): A Xor B = x combine y
}

implicit def xorBifunctor: Bifunctor[Xor] =
new Bifunctor[Xor] {
implicit def xorBifunctor: Bifunctor[Xor] with Bifoldable[Xor] =
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we are combining the Bifunctor and Bifoldable instances in some places but not others. I guess if we are going to go forward with #800 then they will eventually be combined anyway. Was there any particular reason behind when they've been combined and when they haven't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Bifunctor[Xor] instance was there prior to this PR. It wasn't in other places so I figured I would do those separately, hence #867. However because it looks like we have a use case for #800 I may abandon #867 and add Bitraverse once this is OK'ed and merged :-)

@adelbertc
Copy link
Contributor Author

As a note there can also be Bifunctor[Validated] but I'll leave that alone since I'll just define Bitraverse for it in a future PR, unless someone beats me to it :-)

@ceedubs
Copy link
Contributor

ceedubs commented Feb 6, 2016

👍

@ceedubs
Copy link
Contributor

ceedubs commented Feb 6, 2016

Thanks, @adelbertc!

@adelbertc
Copy link
Contributor Author

👍 @stew ? :D

mpilquist
mpilquist reviewed Feb 17, 2016
View reviewed changes
core/src/main/scala/cats/Bifoldable.scala
/**
* A type class abstracting over types that give rise to two independent [[cats.Foldable]]s.
*/
trait Bifoldable[F[_, _]] extends Serializable { self =>
Copy link
Member

Choose a reason for hiding this comment

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

extends Any with Serializable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I see why we would want to do that, but it doesn't seem like we do it in other type classes.. do we?

Copy link
Member

Choose a reason for hiding this comment

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

Simulacrum does it automatically. We probably aren't consistent with the F[_,_] type classes though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright I added it for Bifoldable, I've added an issue to do it for the others and I'll probably end up doing it in a future PR unless someone beats me to it

@mpilquist
Copy link
Member

👍 I didn't see syntax for Bifoldable. Is that intentional?

@adelbertc
Copy link
Contributor Author

Added some more stuff per @mpilquist 's comments - @ceedubs re-review? :D

@ceedubs
Copy link
Contributor

ceedubs commented Feb 17, 2016

👍

Out of curiosity, is the reason we want to extend Any so that it's a universal trait that can be extended by values classes? Are there other reasons?

I could potentially see some value in providing methods to extract a Foldable for the left and a Foldable for the right. These could be handy for checking the Foldable laws for each side and for right-biased types like Xor, ensuring that the right Foldable is consistent with the Foldable instance. However, I don't know if people would find themselves using these in real-world code, so even if we did this maybe they should just reside in tests until someone finds themself wanting them?

@ceedubs
Copy link
Contributor

ceedubs commented Feb 17, 2016

I'll give @mpilquist another chance to review since there were some changes since his last 👍

@mpilquist
Copy link
Member

👍

mpilquist added a commit that referenced this pull request Feb 17, 2016
@mpilquist
Merge pull request #864 from adelbertc/bifoldable
769efe7 
Add Bifoldable, fixes #94
@mpilquist mpilquist merged commit 769efe7 into typelevel:master Feb 17, 2016
peterneyens added a commit to peterneyens/cats that referenced this pull request Apr 11, 2016
@peterneyens
Add syntax for separate from MonadCombine.
60ab295 
I based myself on the structure in syntax.bitraverse for the two implicit conversions (the
existing for unite and the new for separate).

MonadCombine separate was added in typelevel#864 by @adelbertc.
@peterneyens peterneyens mentioned this pull request Apr 11, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adelbertc @codecov-io @ceedubs @mpilquist