Skip to content

Some MonadRec instances. #1041

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
Merged
merged 2 commits into from
May 31, 2016
Merged

Some MonadRec instances. #1041

merged 2 commits into from
May 31, 2016

Conversation

@TomasMikula
Copy link
Contributor

@TomasMikula TomasMikula commented May 17, 2016

Some MonadRec instances to get started. This is a follow-up on #1040.

@TomasMikula TomasMikula mentioned this pull request May 17, 2016
Closed
@TomasMikula TomasMikula force-pushed the MonadRec-instances branch 3 times, most recently from 69355d6 to 85eabda Compare May 17, 2016 02:04
@codecov-io
Copy link

codecov-io commented May 17, 2016 •
edited
Loading

Current coverage is 87.88%

Merging #1041 into master will decrease coverage by 0.45%

  1. 2 files in ...main/scala/cats/data were modified. more
    • Misses -2
    • Hits +3
  2. 1 files in .../src/main/scala/cats were modified. more
  3. 3 files (not in diff) in ...cats/laws/discipline were modified. more
    • Hits -4
  4. 12 files (not in diff) in ...cala/cats/kernel/std were modified. more
  5. 6 files (not in diff) in ...in/scala/cats/kernel were modified. more
    • Misses -3
    • Hits +3
  6. 2 files (not in diff) in ...in/scala/cats/syntax were modified. more
  7. 3 files (not in diff) in .../main/scala/cats/std were modified. more
    • Misses -10
    • Hits +10
  8. 2 files (not in diff) in ...n/scala/cats/functor were modified. more
  9. 3 files (not in diff) in ...main/scala/cats/data were modified. more
    • Misses +3
    • Hits -7
  10. 4 files (not in diff) in .../src/main/scala/cats were modified. more
    • Hits -1
@@             master      #1041   diff @@
==========================================
  Files           215        217     +2   
  Lines          2744       2755    +11   
  Methods        2690       2690          
  Messages          0          0          
  Branches         49         60    +11   
==========================================
- Hits           2424       2421     -3   
- Misses          320        334    +14   
  Partials          0          0

Sunburst

Powered by Codecov. Last updated by 2ab7b93...85eabda

@TomasMikula TomasMikula force-pushed the MonadRec-instances branch from 85eabda to 909a427 Compare May 17, 2016 05:28
adelbertc
adelbertc reviewed May 17, 2016
View reviewed changes
tests/src/test/scala/cats/tests/MonadRecInstancesTests.scala

class MonadRecInstancesTests extends CatsSuite {
def tailRecMStackSafety[M[_]](implicit M: MonadRec[M], Eq: Eq[M[Int]]): Unit = {
val n = 50000
Copy link
Contributor

@adelbertc adelbertc May 17, 2016

Choose a reason for hiding this comment

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

Can this test be added as a MonadRecLaws and MonadRecTests in the cats-laws project, with tests for instances being put in OptionTests, XorTests, etc. ?

Copy link
Contributor

@ceedubs ceedubs May 17, 2016

Choose a reason for hiding this comment

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

A law that tailRecM is consistent with flatMap would be nice.

Copy link
Contributor Author

@TomasMikula TomasMikula May 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@adelbertc Is it OK to call that unit test a law?

@ceedubs The best I can think of is testing that tailRecM is consistent with the naive (non-stack safe) implementation of tailRecM that uses recursive flatMaps (and is implemented uniformly for any monad):

def naiveTailRecM[M[_], A, B](a: A)(f: A => M[A Xor B])(implicit M: Monad[M]): M[B] =
  M.flatMap(f(a))(_ match {
    case Xor.Left(a1) => naiveTailRecM(a1)(f)
    case Xor.Right(b) => M.pure(b)
  })

(This is also basically the implementation of tailRecM for Free, where it is OK, because Free is lazy.)

Did you have something else in mind?

Copy link
Contributor

@ceedubs ceedubs May 17, 2016

Choose a reason for hiding this comment

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

@TomasMikula I was thinking of something like what scalaz has.

Regarding @adelbertc's comment, I have a little hesitation adding something like that as a law. Mostly because it could potentially take a really long time to run for some types. I'm not writing it off as a possibility, just raising a potential concern.

Copy link
Contributor Author

@TomasMikula TomasMikula May 17, 2016

Choose a reason for hiding this comment

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

@ceedubs laws added.

Copy link
Contributor

@ceedubs ceedubs May 17, 2016

Choose a reason for hiding this comment

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

@TomasMikula nice! Thank you for this. I'll do a more detailed review shortly.

@ceedubs
Copy link
Contributor

ceedubs commented May 17, 2016

@mpilquist ScalaDoc generation fails on this line in scala 2.10 but succeeds if I remove the @typeclass annotation. I don't see this listed as a known issue with simulacrum. Any idea what's going on there?

@ceedubs
Copy link
Contributor

ceedubs commented May 17, 2016

Thanks @TomasMikula! This looks like a good start to me.

@TomasMikula TomasMikula force-pushed the MonadRec-instances branch from 909a427 to 2617222 Compare May 17, 2016 21:50
@adelbertc
Copy link
Contributor

adelbertc commented May 18, 2016

Unsure why it's failing but 👍 once we figure that out

@ceedubs
Copy link
Contributor

ceedubs commented May 20, 2016

The build failure seems to be the same as #898 (comment)

@ceedubs ceedubs mentioned this pull request May 21, 2016
Merged
@kailuowang kailuowang mentioned this pull request May 21, 2016
Merged
@TomasMikula
Copy link
Contributor Author

TomasMikula commented May 24, 2016

@adelbertc Do you want to review #1051 which should unblock this one?

@ceedubs
Copy link
Contributor

ceedubs commented May 30, 2016

@TomasMikula #1051 has been merged. Sorry, but would you be willing to rebase?

@TomasMikula TomasMikula force-pushed the MonadRec-instances branch from 2617222 to 4f54045 Compare May 30, 2016 21:40
@non
Copy link
Contributor

non commented May 30, 2016

Great! 👍 when Travis is green.

@TomasMikula TomasMikula force-pushed the MonadRec-instances branch from 4f54045 to 5a38209 Compare May 30, 2016 21:46
@non non merged commit d3c64d1 into typelevel:master May 31, 2016
@ceedubs ceedubs mentioned this pull request May 31, 2016
Closed
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

@TomasMikula @codecov-io @ceedubs @adelbertc @non