Skip to content

Add Bitraverse instances for Validated and XorT.#986

Merged
non merged 1 commit into
typelevel:masterfrom
peterneyens:add-bitraverse-instances
Apr 27, 2016
Merged

Add Bitraverse instances for Validated and XorT.#986
non merged 1 commit into
typelevel:masterfrom
peterneyens:add-bitraverse-instances

Conversation

@peterneyens
Copy link
Copy Markdown
Contributor

@peterneyens peterneyens commented Apr 18, 2016

While experimenting with MonadCombine.separate (for #975), I noticed there were no Bitraverse instances for Validated and XorT.

I would welcome suggestions, possible improvements, ... (maybe for XorTBifoldable ?)

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 18, 2016

Current coverage is 90.85%

Merging #986 into master will increase coverage by +0.03% as of a48a9da

@@            master    #986   diff @@
======================================
  Files          184     184       
  Stmts         2180    2208    +28
  Branches        42      43     +1
  Methods          0       0       
======================================
+ Hit           1980    2006    +26
  Partial          0       0       
- Missed         200     202     +2

Review entire Coverage Diff as of a48a9da

Powered by Codecov. Updated on successful CI builds.

ceedubs
ceedubs reviewed Apr 18, 2016
View reviewed changes
Comment thread core/src/main/scala/cats/data/Validated.scala Outdated
new Bifunctor[Validated] {
override def bimap[A, B, C, D](fab: Validated[A, B])(f: A => C, g: B => D): Validated[C, D] = fab.bimap(f, g)
override def leftMap[A, B, C](fab: Validated[A, B])(f: A => C): Validated[C, B] = fab.leftMap(f)
implicit def validatedBifunctor: Bitraverse[Validated] =
Copy link
Copy Markdown
Contributor

@ceedubs ceedubs Apr 18, 2016

Choose a reason for hiding this comment

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

It might be good to leave the override defs for bimap and leftMap in since they should be slightly more efficient than the versions derived from Bitraverse.

Copy link
Copy Markdown
Contributor

@ceedubs ceedubs Apr 18, 2016

Choose a reason for hiding this comment

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

A couple other notes:

I know this was already a def, but since I noticed it in the diff: could we switch it to a val to avoid repeated allocations? Or does it hit the #965 strangeness?

Are we keeping the name validatedBifunctor for compatibility reasons? If so, we should probably leave a comment that that's what we are doing. However, I think we can feel free to change this to validatedBitraverse since we won't really start to focus on compatibility until a 1.0 release (hopefully coming soon!).

@ceedubs
Copy link
Copy Markdown
Contributor

ceedubs commented Apr 18, 2016

@peterneyens thanks!

In general this looks good. I'd want to see https://github.com/typelevel/cats/pull/986/files#r60056173 addressed before this is merged.

maybe for XorTBifoldable

Sounds good to me!

@peterneyens peterneyens force-pushed the add-bitraverse-instances branch 2 times, most recently from d82cedb to b05b9a1 Compare April 18, 2016 15:06
ceedubs
ceedubs reviewed Apr 18, 2016
View reviewed changes
Comment thread core/src/main/scala/cats/data/XorT.scala Outdated

def bimap[C, D](fa: A => C, fb: B => D)(implicit F: Functor[F]): XorT[F, C, D] = XorT(F.map(value)(_.bimap(fa, fb)))

def bitraverse[G[_], C, D](f: A => G[C], g: B => G[D])(implicit traverseF: Traverse[F], bitraverseXor: Bitraverse[Xor], applicativeG: Applicative[G]): G[XorT[F, C, D]] =
Copy link
Copy Markdown
Contributor

@ceedubs ceedubs Apr 18, 2016

Choose a reason for hiding this comment

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

We shouldn't need to take Bitraverse[Xor] as a parameter, because that's already defined within cats, right?

Copy link
Copy Markdown
Contributor Author

@peterneyens peterneyens Apr 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that felt strange. Can I just write Bitraverse[Xor].bitraverse(axb)(f, g) directly ?
Can I do the same with Bifoldable[Xor] in XorTBifoldable ?

Copy link
Copy Markdown
Contributor

@ceedubs ceedubs Apr 18, 2016

Choose a reason for hiding this comment

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

Seems reasonable to me. I don't know why we have traverseXorA in the XorT.traverse signature. I would think we could remove that too.

@peterneyens peterneyens force-pushed the add-bitraverse-instances branch from b05b9a1 to 9efc288 Compare April 18, 2016 19:40
@ceedubs
Copy link
Copy Markdown
Contributor

ceedubs commented Apr 18, 2016

Thanks again @peterneyens, especially for the cleanup you did outside of your initial PR. 👍 as long as the build goes green.

@Jacoby6000
Copy link
Copy Markdown
Contributor

Jacoby6000 commented Apr 19, 2016

If #976 gets merged, it gives you a bitraverse for these types for free (but I didn't clean things up)

@non
Copy link
Copy Markdown
Contributor

non commented Apr 27, 2016

This looks good to me. 👍

Unfortunately there are merge conflicts now. Sorry!

@peterneyens would you be able to merge and update? If not, I can give it a shot (I imagine the merge will be relatively easy).

@peterneyens peterneyens force-pushed the add-bitraverse-instances branch from 9efc288 to da49118 Compare April 27, 2016 15:26
@peterneyens
Copy link
Copy Markdown
Contributor Author

peterneyens commented Apr 27, 2016

It was a simple merge. The conflict was just me renaming xorBifunctor to xorBitraverse.

@non
Copy link
Copy Markdown
Contributor

non commented Apr 27, 2016

Works for me. 👍

@non non merged commit 95501f7 into typelevel:master Apr 27, 2016
@peterneyens peterneyens deleted the add-bitraverse-instances branch April 27, 2016 16:28
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.

5 participants

@peterneyens @codecov-io @ceedubs @Jacoby6000 @non