cats-effect module #155

BenFradet · 2017-05-30T17:49:44Z

js support
remove hardcoded cats-effect version once sbt-org-policies is out
doc

BenFradet

Reviews/help welcome 👍

BenFradet · 2017-05-30T17:51:41Z

cats-effect/src/main/scala/github4s/cats/effect/Implicits.scala

+import github4s.free.interpreters.{Capture, Interpreters}
+import scalaj.http.HttpResponse
+
+object implicits extends HttpRequestBuilderExtensionJVM {


should I create one implicits for the jvm and one for js?

By now, I think that you should. Nevertheless, I guess we want to get rid of this kind of implicit import.

#45

One option would use a fully compatible http client for both jvm and js: https://github.com/pepegar/hammock .

What do you think?

in the long run I think this would be great indeed.

welp I think I see what you mean since roshttp is tied to Future we can't support IO in scala js

BenFradet · 2017-05-30T17:52:13Z

build.sbt

+lazy val catsEffect = (project in file("cats-effect"))
+  .settings(moduleName := "github4s-cats-effect")
+  .settings(catsEffectDependencies: _*)
+  .dependsOn(github4sJVM)


is it ok to depend on both github4sJVM and github4sJS?

I don't know if cat-effects is already a crossed-project with Scalajs (but it will be likely). If that's already the case, we could make catsEffect module as crossProject too.

yes it supports scala js, I'll try making it a cross project

BenFradet · 2017-05-30T17:52:55Z

project/ProjectPlugin.scala

+
+    lazy val catsEffectDependencies: Def.Setting[Seq[ModuleID]] =
+      libraryDependencies ++= Seq(
+        "org.typelevel" %% "cats-effect" % "0.3",


codecov-io · 2017-05-31T00:51:30Z

Codecov Report

Merging #155 into master will increase coverage by 0.03%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
+ Coverage   86.92%   86.95%   +0.03%     
==========================================
  Files          36       40       +4     
  Lines         566      575       +9     
  Branches        2        1       -1     
==========================================
+ Hits          492      500       +8     
- Misses         74       75       +1

Impacted Files	Coverage Δ
...cala/github4s/HttpRequestBuilderExtensionJVM.scala	`100% <ø> (ø)`	⬆️
.../scala/github4s/cats/effect/jvm/ImplicitsJVM.scala	`100% <100%> (ø)`
...in/scala/github4s/cats/effect/js/ImplicitsJS.scala	`100% <100%> (ø)`
...scala/github4s/cats/effect/IOCaptureInstance.scala	`100% <100%> (ø)`
.../cats/effect/IOHttpRequestBuilderExtensionJS.scala	`75% <75%> (ø)`
...scala/github4s/HttpRequestBuilderExtensionJS.scala	`81.48% <92.85%> (+0.71%)`	⬆️

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 583aa1a...c3690d2. Read the comment docs.

juanpedromoreno · 2017-05-31T14:29:17Z

build.sbt

+lazy val catsEffect = (project in file("cats-effect"))
+  .settings(moduleName := "github4s-cats-effect")
+  .settings(catsEffectDependencies: _*)
+  .dependsOn(github4sJVM)


I don't know if cat-effects is already a crossed-project with Scalajs (but it will be likely). If that's already the case, we could make catsEffect module as crossProject too.

juanpedromoreno · 2017-05-31T14:32:47Z

cats-effect/src/main/scala/github4s/cats/effect/Implicits.scala

+import github4s.free.interpreters.{Capture, Interpreters}
+import scalaj.http.HttpResponse
+
+object implicits extends HttpRequestBuilderExtensionJVM {


By now, I think that you should. Nevertheless, I guess we want to get rid of this kind of implicit import.

#45

One option would use a fully compatible http client for both jvm and js: https://github.com/pepegar/hammock .

What do you think?

juanpedromoreno · 2017-05-31T14:34:42Z

project/ProjectPlugin.scala

+
+    lazy val catsEffectDependencies: Def.Setting[Seq[ModuleID]] =
+      libraryDependencies ++= Seq(
+        "org.typelevel" %% "cats-effect" % "0.3",


sbt-org-policies 0.5.2 has been already released: https://github.com/47deg/sbt-org-policies/releases/tag/v0.5.2

juanpedromoreno · 2017-05-31T14:44:50Z

BTW I don't know why my review was sent twice XD

BenFradet · 2017-05-31T18:28:53Z

Since it seems we can't use a Capture with HttpRequestBuilderExtensionJS, we can either:

build a HttpRequestBuilderExtensionJS specifically for IO in cats-effect js
not support cats-effect in js for the moment (until we port everything to hammock)

What do you think?

juanpedromoreno · 2017-05-31T19:45:20Z

Makes sense 👍

BenFradet · 2017-06-01T18:36:33Z

cats-effect/js/src/test/scala/github4s/cats/effect/js/CatsEffectJSSpec.scala

+    val res = response.unsafeRunSync
+    res.isLeft shouldBe true
+  }
+}


This spec is never run, I don't know why.

BenFradet · 2017-06-07T17:55:17Z

cats-effect/js/src/test/scala/github4s/cats/effect/js/CatsEffectJSSpec.scala

+        }
+        .getOrElse(fail("effect attempt failed"))
+    }
+  }


curious as to how you found all this?

cats-effects project, I only adapted it for our use case ;)

BenFradet · 2017-06-07T19:08:04Z

docs/src/main/tut/docs.md

+### Using `cats.effect.IO`
+
+On the JVM:
+```scala


I can't seem to get this example to compile with tut despite adding cats-effect to the doc deps, any ideas?

juanpedromoreno · 2017-06-08T12:39:07Z

cats-effect/js/src/test/scala/github4s/cats/effect/js/CatsEffectJSSpec.scala

+        }
+        .getOrElse(fail("effect attempt failed"))
+    }
+  }


cats-effects project, I only adapted it for our use case ;)

juanpedromoreno · 2017-06-08T13:05:56Z

docs/src/main/tut/docs.md

+### Using `cats.effect.IO`
+
+On the JVM:
+```scala


juanpedromoreno · 2017-06-08T13:07:01Z

project/ProjectPlugin.scala

      ),
-      coverageExcludedPackages := "<empty>;github4s\\.scalaz\\..*",
+      coverageExcludedPackages := "<empty>;github4s\\.scalaz\\..*;github4s\\.cats\\.effect\\..*",


Why are we excluding these tests?

Just thought I'd exclude them like what is being done for scalaz, happy to include them.

👍

scalaz doesn't have tests, cats-effect has thanks to your work :)

BenFradet · 2017-06-08T17:25:43Z

mmh I get 100% coverage on cats-effect-jvm but 78.57% on js, because of the HttpRequestBuilder for IO, I'll try adding tests for that as well.

juanpedromoreno · 2017-06-08T21:17:13Z

Ohh, thanks. Let's try it, if not, we could remove the 80% restriction.

BenFradet · 2017-06-08T21:30:05Z

Yup, I'll try tomorrow.

Anyway the low coverage is only temporary since once we move to hammock there'll be no HttpRequestBuilder extension for IO and the capture instance will be used in JS so we'll be at 💯 for both JVM and JS.

BenFradet · 2017-06-09T18:14:16Z

@juanpedromoreno I think it's ready for a review 👍

juanpedromoreno

Looks great to me, thanks! Outstanding job

BenFradet · 2017-06-10T12:53:27Z

Thanks a lot for your help, merging!

cats-effect module

2b04152

BenFradet commented May 30, 2017

View reviewed changes

juanpedromoreno reviewed May 31, 2017

View reviewed changes

js support

e1a52d6

BenFradet commented Jun 1, 2017

View reviewed changes

BenFradet and others added 4 commits June 1, 2017 19:38

excluding cats-effect from code cov

2d086a3

Fixes sbt dependencies for cats-effects module

99c963b

Fixes cats-effect tests for scalajs

5eec899

docs

db0780c

BenFradet commented Jun 7, 2017

View reviewed changes

BenFradet changed the title ~~[wip] cats-effect module~~ cats-effect module Jun 7, 2017

BenFradet mentioned this pull request Jun 7, 2017

Migrate http requests to hammock #157

Closed

juanpedromoreno reviewed Jun 8, 2017

View reviewed changes

juanpedromoreno and others added 2 commits June 8, 2017 14:28

Fixes cats-effect docs

d222b52

cats-effect taken into account during code coverage

8226339

dummy test to increase coverage

c3690d2

BenFradet force-pushed the bf-cats-effect branch from af9d958 to c3690d2 Compare June 9, 2017 17:35

juanpedromoreno approved these changes Jun 10, 2017

View reviewed changes

BenFradet merged commit f59e2de into master Jun 10, 2017

BenFradet deleted the bf-cats-effect branch June 10, 2017 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cats-effect module #155

cats-effect module #155

BenFradet commented May 30, 2017 •

edited

BenFradet left a comment

BenFradet May 30, 2017

juanpedromoreno May 31, 2017

BenFradet May 31, 2017

BenFradet May 31, 2017

BenFradet May 30, 2017

juanpedromoreno May 31, 2017

BenFradet May 31, 2017

BenFradet May 30, 2017

codecov-io commented May 31, 2017 •

edited

juanpedromoreno May 31, 2017

juanpedromoreno May 31, 2017

juanpedromoreno May 31, 2017

juanpedromoreno commented May 31, 2017

BenFradet commented May 31, 2017

juanpedromoreno commented May 31, 2017

BenFradet Jun 1, 2017

juanpedromoreno Jun 6, 2017

BenFradet Jun 7, 2017

juanpedromoreno Jun 8, 2017

BenFradet Jun 7, 2017

juanpedromoreno Jun 8, 2017

juanpedromoreno Jun 8, 2017

juanpedromoreno Jun 8, 2017

juanpedromoreno Jun 8, 2017

BenFradet Jun 8, 2017

juanpedromoreno Jun 8, 2017

BenFradet commented Jun 8, 2017

juanpedromoreno commented Jun 8, 2017

BenFradet commented Jun 8, 2017

BenFradet commented Jun 9, 2017

juanpedromoreno left a comment

BenFradet commented Jun 10, 2017

cats-effect module #155

cats-effect module #155

Conversation

BenFradet commented May 30, 2017 • edited

BenFradet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 31, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanpedromoreno commented May 31, 2017

BenFradet commented May 31, 2017

juanpedromoreno commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenFradet commented Jun 8, 2017

juanpedromoreno commented Jun 8, 2017

BenFradet commented Jun 8, 2017

BenFradet commented Jun 9, 2017

juanpedromoreno left a comment

Choose a reason for hiding this comment

BenFradet commented Jun 10, 2017

BenFradet commented May 30, 2017 •

edited

codecov-io commented May 31, 2017 •

edited