Add syntax for Bitraverse to allow traversal and sequencing of the left side #2606

andimiller · 2018-11-12T18:01:37Z

This adds Bitraverse-based syntax for:

leftTraverse
leftSequence

This adds syntax for: * traverse * sequence * leftTraverse * leftSequence I am open to having better names, but I think these would be useful to have in cats-core.

tests/src/test/scala/cats/tests/SyntaxSuite.scala

LukaJCB · 2018-11-12T18:58:58Z

core/src/main/scala/cats/syntax/bitraverse.scala

 }

 final class BitraverseOps[F[_, _], A, B](val fab: F[A, B]) extends AnyVal {
  def bitraverse[G[_]: Applicative, C, D](f: A => G[C], g: B => G[D])(implicit F: Bitraverse[F]): G[F[C, D]] =
    F.bitraverse(fab)(f, g)
+  def traverse[G[_], C](f: B => G[C])(implicit F: Bitraverse[F], G: Applicative[G]): G[F[A, C]] =


I'm not sure about this one and sequence, we already have them defined on Traverse and I think that can cause complications. Maybe we should remove these or rename them to rightTraverse and rightSequence

I wasn't sure about this, but I'll change it to rightTraverse and rightSequence for now. You don't normally have syntax in scope to give you traverse and sequence, so I do think they're still valuable to have.

LukaJCB

Thanks a ton for opening this, I left a couple of comments :)

…equence

kubukoz · 2018-11-13T11:57:31Z

Looks like this isn't binary compatible, @andimiller would you mind adding these implicit conversions to a new trait like https://github.com/typelevel/cats/pull/2600/files ?

andimiller · 2018-11-13T13:31:43Z

Looks like this isn't binary compatible, @andimiller would you mind adding these implicit conversions to a new trait like https://github.com/typelevel/cats/pull/2600/files ?

I have no idea how binary compatibility works, but I can give it a go copying how that PR does it

codecov-io · 2018-11-13T13:59:42Z

Codecov Report

Merging #2606 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2606      +/-   ##
==========================================
+ Coverage   95.13%   95.15%   +0.02%     
==========================================
  Files         365      365              
  Lines        6757     6773      +16     
  Branches      286      309      +23     
==========================================
+ Hits         6428     6445      +17     
+ Misses        329      328       -1

Impacted Files	Coverage Δ
core/src/main/scala/cats/syntax/bitraverse.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/duration.scala	`100% <0%> (ø)`	⬆️
...src/main/scala/cats/instances/finiteDuration.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/syntax/monadError.scala	`100% <0%> (ø)`	⬆️
...estkit/src/main/scala/cats/tests/ListWrapper.scala	`100% <0%> (ø)`	⬆️
testkit/src/main/scala/cats/tests/CatsSuite.scala	`70% <0%> (ø)`	⬆️
core/src/main/scala/cats/data/Chain.scala	`99.6% <0%> (+0.01%)`	⬆️
core/src/main/scala/cats/data/EitherT.scala	`97.91% <0%> (+0.01%)`	⬆️
kernel/src/main/scala/cats/kernel/Hash.scala	`100% <0%> (+8.33%)`	⬆️

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 ee7d356...28f6c9a. Read the comment docs.

andimiller · 2018-11-13T14:04:23Z

I think I've managed to make it binary compatible, I've left {Left,Right}NestedBitraverseOps without the BinCompat0 suffix because they're new syntax classes, not sure if they should have the suffix to match the others, or if it's okay to leave them as is, because there are no "original" versions of those classes?

…tax.all

andimiller · 2018-11-13T15:05:52Z

Should I have made a new SyntaxBinCompat4 or put it into an existing one? No idea how these are meant to work

kubukoz · 2018-11-13T15:09:49Z

I believe it'll be fine if you add it to bincompat3. As long as you get this merged before 1.5.0 is released :)

ikempf · 2018-11-30T10:24:28Z

I think there should be no rightTraverse nor rightSequence since both are already offered by traverse and sequence from Traverse typeclass.

Bifunctor only offers bimap and leftMap since map/rightMap is already offered by Functor.

andimiller · 2018-12-03T17:18:51Z

I've changed this to only provide leftTraverse and leftSequence, since adding the right-based versions is out of scope for this PR

ikempf · 2018-12-22T05:17:07Z

@andimiller Hi, are you still on this ? Now that 1.5.0 is out this will have to go into a bincompat4.

andimiller · 2018-12-22T06:53:45Z

@ikempf yeah I'll be updating this PR later today to put it in bincompat4

LukaJCB

Thanks for keeping this up to date :)

LukaJCB · 2019-01-08T18:23:16Z

Seems like there's a bug in the travis reporting, I'm going to close/reopen :)

kailuowang · 2019-01-09T19:49:59Z

Thanks! @andimiller Adding some doctests, an example, would be helpful. Although I admit that a doctest here would be hard to find. I'll leave that to you.

andimiller · 2019-01-10T12:00:56Z

I've added doctests to these syntax methods, I did look at adding these methods directly onto Bitraverse as derived methods, but that trips the binary compatibility checker, so it'd probably need to wait til 2.0.

I also can't figure out how to run doctests, so I've pushed it to see if travis manages it better than I can locally.

kailuowang · 2019-01-10T13:22:23Z

@andimiller thanks for adding them. simple test will run the doctest. In this case sbt coreJVM/test

andimiller added 2 commits November 12, 2018 17:39

Add syntax for Bitraverse to allow independent traversal of sides

94626ed

This adds syntax for: * traverse * sequence * leftTraverse * leftSequence I am open to having better names, but I think these would be useful to have in cats-core.

Add syntax tests and accept prePR's style change

07f3276

LukaJCB reviewed Nov 12, 2018

View reviewed changes

tests/src/test/scala/cats/tests/SyntaxSuite.scala Outdated Show resolved Hide resolved

LukaJCB reviewed Nov 12, 2018

View reviewed changes

andimiller added 2 commits November 12, 2018 21:09

Fix repeated variables in SyntaxSuite

fe293fb

Rename bitraverse's traverse and sequence to rightTraverse and rightS…

cf54795

…equence

Attempt to make new Bitraverse syntax binary compatible

15f6f9c

Add a new syntax bincompat to let me add new Bitraverse syntax to syn…

8e1332c

…tax.all

andimiller added 2 commits November 13, 2018 15:31

Move Bitraverse new syntax into AllSyntaxBinCompat3

a3414c8

Merge branch 'master' into feature/bitraverse-independent-traversal

5048c64

LukaJCB mentioned this pull request Nov 29, 2018

Add leftTraverse and leftSequence to Bitraverse typeclass #2646

Closed

Drop rightTraverse and rightSequence from this PR

ad87d8e

andimiller changed the title ~~Add syntax for Bitraverse to allow independent traversal of sides~~ Add syntax for Bitraverse to allow traversal and sequencing of the left side Dec 3, 2018

andimiller added 3 commits December 22, 2018 17:45

Freshen from master, move to bincompat4

6c67bb3

scalafmt

558b372

Merge branch 'master' into feature/bitraverse-independent-traversal

ace84de

LukaJCB previously approved these changes Jan 8, 2019

View reviewed changes

kailuowang added this to the 1.6 milestone Jan 8, 2019

LukaJCB closed this Jan 8, 2019

LukaJCB reopened this Jan 8, 2019

kailuowang previously approved these changes Jan 9, 2019

View reviewed changes

Add scaladoc on leftSequence/leftTraverse

76d8787

andimiller dismissed stale reviews from kailuowang and LukaJCB via 76d8787 January 10, 2019 11:58

andimiller added 3 commits January 10, 2019 12:09

Fix doctests for leftTraverse/leftSequence

14bbe68

Fix doctests for leftTraverse/leftSequence again

4b05510

Fix doctests for leftTraverse/leftSequence yet again

28f6c9a

kailuowang approved these changes Jan 11, 2019

View reviewed changes

kailuowang merged commit b4d1ad1 into typelevel:master Jan 11, 2019

kailuowang added the enhancement label Jan 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add syntax for Bitraverse to allow traversal and sequencing of the left side #2606

Add syntax for Bitraverse to allow traversal and sequencing of the left side #2606

andimiller commented Nov 12, 2018 •

edited

Loading

LukaJCB Nov 12, 2018

andimiller Nov 12, 2018

LukaJCB left a comment

kubukoz commented Nov 13, 2018

andimiller commented Nov 13, 2018

codecov-io commented Nov 13, 2018 •

edited

Loading

andimiller commented Nov 13, 2018

andimiller commented Nov 13, 2018

kubukoz commented Nov 13, 2018 •

edited

Loading

ikempf commented Nov 30, 2018 •

edited

Loading

andimiller commented Dec 3, 2018

ikempf commented Dec 22, 2018

andimiller commented Dec 22, 2018

LukaJCB left a comment

LukaJCB commented Jan 8, 2019

kailuowang commented Jan 9, 2019

andimiller commented Jan 10, 2019

kailuowang commented Jan 10, 2019

Add syntax for Bitraverse to allow traversal and sequencing of the left side #2606

Add syntax for Bitraverse to allow traversal and sequencing of the left side #2606

Conversation

andimiller commented Nov 12, 2018 • edited Loading

LukaJCB Nov 12, 2018

Choose a reason for hiding this comment

andimiller Nov 12, 2018

Choose a reason for hiding this comment

LukaJCB left a comment

Choose a reason for hiding this comment

kubukoz commented Nov 13, 2018

andimiller commented Nov 13, 2018

codecov-io commented Nov 13, 2018 • edited Loading

Codecov Report

andimiller commented Nov 13, 2018

andimiller commented Nov 13, 2018

kubukoz commented Nov 13, 2018 • edited Loading

ikempf commented Nov 30, 2018 • edited Loading

andimiller commented Dec 3, 2018

ikempf commented Dec 22, 2018

andimiller commented Dec 22, 2018

LukaJCB left a comment

Choose a reason for hiding this comment

LukaJCB commented Jan 8, 2019

kailuowang commented Jan 9, 2019

andimiller commented Jan 10, 2019

kailuowang commented Jan 10, 2019

andimiller commented Nov 12, 2018 •

edited

Loading

codecov-io commented Nov 13, 2018 •

edited

Loading

kubukoz commented Nov 13, 2018 •

edited

Loading

ikempf commented Nov 30, 2018 •

edited

Loading