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

adding helper method toFreeT to Free #2724

Merged
merged 7 commits into from
Feb 27, 2019
Merged

Conversation

jcouyang
Copy link
Contributor

@jcouyang jcouyang commented Feb 4, 2019

so any Free[S, A] can be converted to FreeT[S, G, A] if there is an Applicative instance of G

@ceedubs
Copy link
Contributor

ceedubs commented Feb 9, 2019

Thanks @jcouyang! It looks like scalafmt is complaining about some whitespace. If you run sbt prePR it should resolve the issue.

@jcouyang
Copy link
Contributor Author

@ceedubs I ran sbt prePR and everything was ok, weird, just ran sbt scalafmt see if it will fix the format

@codecov-io
Copy link

codecov-io commented Feb 10, 2019

Codecov Report

Merging #2724 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2724      +/-   ##
==========================================
+ Coverage   95.13%   95.14%   +0.01%     
==========================================
  Files         365      365              
  Lines        6798     6818      +20     
  Branches      296      302       +6     
==========================================
+ Hits         6467     6487      +20     
  Misses        331      331
Impacted Files Coverage Δ
free/src/main/scala/cats/free/Free.scala 92.2% <100%> (+0.2%) ⬆️
...ts/laws/discipline/InvariantSemigroupalTests.scala 100% <0%> (ø) ⬆️
.../cats/laws/discipline/InvariantMonoidalTests.scala 100% <0%> (ø) ⬆️
.../scala/cats/laws/discipline/ArrowChoiceTests.scala 100% <0%> (ø) ⬆️
...ain/scala/cats/laws/InvariantSemigroupalLaws.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/MonadError.scala 100% <0%> (ø) ⬆️
.../main/scala/cats/laws/discipline/StrongTests.scala 100% <0%> (ø) ⬆️
.../src/main/scala/cats/syntax/applicativeError.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Kleisli.scala 97.29% <0%> (ø) ⬆️
laws/src/main/scala/cats/laws/StrongLaws.scala 100% <0%> (ø) ⬆️
... and 3 more

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 356e339...8b56092. Read the comment docs.

ceedubs
ceedubs previously approved these changes Feb 15, 2019
Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

👍 thanks @jcouyang!

LukaJCB
LukaJCB previously approved these changes Feb 15, 2019
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Looks good :)

new (S ~> FreeT[S, G, ?]) {
def apply[B](fa: S[B]): FreeT[S, G, B] = FreeT.liftF(fa)
}
}(FreeT.catsFreeMonadForFreeT[S, G])
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably be simplified as

foldMap( λ[S ~> FreeT[S, G, ?]](FreeT.lift(_))

@@ -45,6 +45,10 @@ class FreeSuite extends CatsSuite {
rr.toString.length should be > 0
}

test("toFreeT is stack-safe") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this one down to be grouped with the other stack safety test?

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Thanks! I added some minor comments

@jcouyang jcouyang dismissed stale reviews from LukaJCB and ceedubs via 32df4d0 February 17, 2019 07:37
@jcouyang
Copy link
Contributor Author

thanks @kailuowang, comment addressed, please have a look :)

@kailuowang
Copy link
Contributor

so just foldMap(λ[S ~> FreeT[S, G, ?]](FreeT.lift(_)) (without the explicit type and instance param) wouldn't compile for you?

@jcouyang
Copy link
Contributor Author

🤔 not sure why it works now...looks like λ magic
the compile can't find the correct monad instance when it was new (S ~> FreeT[S, Id, ?]) before so I explicitly specify that a FreeT Monad for S and G

anyway, removing the explicit paramter in mapK too

@jcouyang
Copy link
Contributor Author

hi @kailuowang can you please help review the latest change?

@LukaJCB
Copy link
Member

LukaJCB commented Feb 26, 2019

Hey @jcouyang, sorry for not getting to this earlier. Kai had his phone stolen on vacation and I've been away as well. Anyway, this looks good to me :) Will merge this soon!

@jcouyang
Copy link
Contributor Author

🙀 oh no, sorry to hear that

@kailuowang kailuowang merged commit 2ad9901 into typelevel:master Feb 27, 2019
@kailuowang
Copy link
Contributor

@jcouyang thanks a lot! as Luka mentioned, sorry about the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants