Merge plugins #43

ememmr · 2020-05-20T09:19:42Z

Issue related: https://github.com/47deg/marlow/issues/635

# Conflicts: # project/ProjectPlugin.scala

BenFradet

👍

juanpedromoreno · 2020-05-22T05:57:33Z

plugin/src/main/scala/higherkindness/mu/rpc/srcgen/SrcGenPlugin.scala

+
+    lazy val muSrcGenCompendiumServerPort: SettingKey[Int] =
+      settingKey[Int]("Port of the compendium server")
+


suggestion: I think it worth specifying what are the default values for every new setting. Thoughts?

I have to update documentation too, which has (or should have) the default values. But it's not a bad idea

juanpedromoreno · 2020-05-22T06:03:07Z

project/ProjectPlugin.scala

@@ -17,6 +17,7 @@ object ProjectPlugin extends AutoPlugin {
    lazy val V = new {
      val avrohugger: String          = "1.0.0-RC22"
      val circe: String               = "0.13.0"
+      val hammock                     = "0.10.0"


Hammock 0.10.0 is bringing circe 0.12.1, as you can see here. However, that will provoke a collision with the circe version we are using in this build: 0.13.0.

My suggestion would be using an http4s client, as it is evolving at the same time as typelevel ecosystem does. The other option would be to work on Hammock and try to make it updated and released with the latest dependency versions.

ready, please take a look @juanpedromoreno

juanpedromoreno · 2020-05-26T08:46:42Z

project/ProjectPlugin.scala

@@ -25,6 +25,7 @@ object ProjectPlugin extends AutoPlugin {
      val scalatestplusScheck: String = "3.1.0.0-RC2"
      val skeuomorph: String          = "0.0.23"
      val slf4j: String               = "1.7.30"
+      val http4s: String              = "0.21.1"


Suggested change

val http4s: String = "0.21.1"

val http4s: String = "0.21.4"

I'll put all the changes in the same commit

juanpedromoreno · 2020-05-26T08:49:49Z

core/src/main/scala/higherkindness/mu/rpc/srcgen/compendium/CompendiumClient.scala

-      F: Sync[F]
+  def apply[F[_]: Sync](
+      clientF: Client[F],
+      clientConfig: HttpConfig
  ): CompendiumClient[F] =
    new CompendiumClient[F] {

      val baseUrl: String          = s"http://${clientConfig.host}:${clientConfig.port}"


suggestion: I'd parametrize the http part of the Url (it could be https).

Or merge both in a single property called "connectionUrl" or something like that

Yeah I would put the whole URL, including the port number, into a single property (connectionUrl? serverUrl?). Fewer things for the user to configure, and no assumptions about how they want to host the server.

for now, it could be merged. It was split because we had, at least, two endpoints to call.

So, does this work for both of you?

val versionParam = version.fold("")(v => s"?version=${v.show}") val connectionUrl = s"https://${clientConfig.host}:${clientConfig.port}/v0/protocol/$identifier$versionParam"

Sorry, I wasn't very clear. My suggestion was to have the server's base URL (e.g. https://devtools.mycorp.com:1234/compendium) passed in as a single property inside HttpConfig. Inside this method we would construct the full URL:

val connectionUrl = s"${clientConfig.serverUrl}/v0/protocol/$identifier$versionParam"

juanpedromoreno · 2020-05-26T08:51:06Z

core/src/main/scala/higherkindness/mu/rpc/srcgen/compendium/CompendiumClient.scala

-            case Status.OK       => request.as[RawProtocol].map(Option(_)).exec[F]
+        clientF.get(uri)(res =>
+          res.status match {
+            case Status.Ok       => res.as[RawProtocol].map(Option(_))
            case Status.NotFound => Sync[F].pure(None)


question: If something is not found, it worth to notify the user?

With the current development, it raises an error:

case None => Async[F].raiseError[File]( ProtocolNotFound(s"Protocol ${protocolAndVersion.name} not found in Compendium. ") )

on run function

I think it makes sense to just return None at this point, because we are just an HTTP client and we don't know what we are being used for. It's pretty reasonable to try to get a resource which might not exist, so it's not an exceptional circumstance.

Whoever is using the client can decide whether a missing resource should be an error or not.

juanpedromoreno · 2020-05-26T08:52:53Z

core/src/main/scala/higherkindness/mu/rpc/srcgen/compendium/CompendiumMode.scala


 final case class ProtocolAndVersion(name: String, version: Option[String])
 final case class FilePrintWriter(file: File, pw: PrintWriter)

-case class CompendiumMode[F[_]: Async](
+case class CompendiumMode[F[_]: Async: ConcurrentEffect](


Suggested change

case class CompendiumMode[F[_]: Async: ConcurrentEffect](

final case class CompendiumMode[F[_]: ConcurrentEffect](

juanpedromoreno · 2020-05-26T08:55:04Z

core/src/main/scala/higherkindness/mu/rpc/srcgen/compendium/CompendiumMode.scala

@@ -1,46 +1,52 @@
 package higherkindness.mu.rpc.srcgen.compendium

 import java.io.{File, PrintWriter}
-import cats.effect.{Async, Resource, Sync}
+
+import cats.effect.{Async, ConcurrentEffect, Resource, Sync}


Suggested change

import cats.effect.{Async, ConcurrentEffect, Resource, Sync}

import cats.effect.{Resource, ConcurrentEffect}

I think you have a mix of effects here, but I believe that you are just requesting ConcurrentEffect for the client resource usage.

juanpedromoreno · 2020-05-26T08:57:35Z

core/src/main/scala/higherkindness/mu/rpc/srcgen/compendium/CompendiumMode.scala

+                path = path
+              )
+            case None =>
+              Async[F].raiseError[File](


Suggested change

Async[F].raiseError[File](

ConcurrentEffect[F].raiseError[File](

juanpedromoreno · 2020-05-26T08:58:59Z

core/src/main/scala/higherkindness/mu/rpc/srcgen/compendium/CompendiumMode.scala

+              )
+          }
+        } yield file
+      })


You will need to replace the Sync[F] below by ConcurrentEffect[F] (writeTempFile function).

cb372 · 2020-05-27T10:53:28Z

core/src/main/scala/higherkindness/mu/rpc/srcgen/compendium/CompendiumClient.scala

+   * @param version    optional protocol version number
+   * @return a protocol
+   */
+  def retrieveProtocol(identifier: String, version: Option[Int]): F[Option[RawProtocol]]


Should we make the version a String or something, in anticipation of higherkindness/compendium#284 ?

juanpedromoreno · 2020-05-28T08:33:12Z

plugin/src/main/scala/higherkindness/mu/rpc/srcgen/SrcGenPlugin.scala

+      )
+
+    lazy val muSrcGenCompendiumServerUrl: SettingKey[String] =
+      settingKey[String]("Url of the compendium server. By default, `http://localhost:47047`.")


Suggested change

settingKey[String]("Url of the compendium server. By default, `http://localhost:47047`.")

settingKey[String]("Url of the compendium server. By default, `https://localhost:47047`.")

juanpedromoreno · 2020-05-28T08:33:37Z

project/ProjectPlugin.scala

@@ -17,13 +17,15 @@ object ProjectPlugin extends AutoPlugin {
    lazy val V = new {
      val avrohugger: String          = "1.0.0-RC22"
      val circe: String               = "0.13.0"
+      val hammock                     = "0.10.0"


Suggested change

val hammock = "0.10.0"

cb372 · 2020-05-28T09:08:45Z

plugin/src/main/scala/higherkindness/mu/rpc/srcgen/SrcGenPlugin.scala

+    muSrcGenStreamingImplementation := Fs2Stream,
+    muSrcGenExecutionMode := Local,
+    muSrcGenCompendiumProtocolIdentifiers := Nil,
+    muSrcGenCompendiumServerUrl := "https://localhost:47047"


Is the server likely to use HTTPS if it's running on localhost?

ok, just checked aaaand nop. At least not by default 😞

juanpedromoreno · 2020-05-28T11:13:18Z

plugin/src/main/scala/higherkindness/mu/rpc/srcgen/SrcGenPlugin.scala

+      )
+
+    lazy val muSrcGenCompendiumServerUrl: SettingKey[String] =
+      settingKey[String]("Url of the compendium server. By default, `https://localhost:47047`.")


Suggested change

settingKey[String]("Url of the compendium server. By default, `https://localhost:47047`.")

settingKey[String]("Url of the compendium server. By default, `http://localhost:47047`.")

juanpedromoreno · 2020-05-28T11:13:31Z

plugin/src/main/scala/higherkindness/mu/rpc/srcgen/SrcGenPlugin.scala

+    muSrcGenStreamingImplementation := Fs2Stream,
+    muSrcGenExecutionMode := Local,
+    muSrcGenCompendiumProtocolIdentifiers := Nil,
+    muSrcGenCompendiumServerUrl := "https://localhost:47047"


Suggested change

muSrcGenCompendiumServerUrl := "https://localhost:47047"

muSrcGenCompendiumServerUrl := "http://localhost:47047"

fedefernandez · 2020-05-28T11:17:03Z

core/src/main/scala/higherkindness/mu/rpc/srcgen/compendium/CompendiumClient.scala

+            case Status.InternalServerError =>
+              Sync[F].raiseError(UnknownError(s"Error in compendium server"))
+            case _ =>
+              Sync[F].raiseError(UnknownError(s"Unknown error with status code ${res.status.code}"))


Suggested change

case Status.InternalServerError =>

Sync[F].raiseError(UnknownError(s"Error in compendium server"))

case _ =>

Sync[F].raiseError(UnknownError(s"Unknown error with status code ${res.status.code}"))

case s => Sync[F].raiseError(UnexpectedStatus(s))

https://github.com/http4s/http4s/blob/698e7a2f2aeed71e13439f24f36772ec4901e7d6/client/src/main/scala/org/http4s/client/Client.scala#L246

fedefernandez · 2020-05-28T11:17:50Z

core/src/main/scala/higherkindness/mu/rpc/srcgen/compendium/CompendiumMode.scala

+              ConcurrentEffect[F].raiseError[File](
+                ProtocolNotFound(s"Protocol ${protocolAndVersion.name} not found in Compendium. ")
+              )


Suggested change

ConcurrentEffect[F].raiseError[File](

ProtocolNotFound(s"Protocol ${protocolAndVersion.name} not found in Compendium. ")

)

ProtocolNotFound(s"Protocol ${protocolAndVersion.name} not found in Compendium. ")

.raiseError[F, File]

mrtmmr added 5 commits May 7, 2020 12:19

Create compendium mode execution

94e3c1b

Merge branch 'master' into merge-plugins

f031114

# Conflicts: # project/ProjectPlugin.scala

Scala fmtr

9b35fa9

Merge branch 'master' into merge-plugins

44cf673

scalafmtrAll

360a5fd

BenFradet approved these changes May 20, 2020

View reviewed changes

mrtmmr added 3 commits May 20, 2020 11:46

Test default mode

e006325

Create directory

dec76ce

Change execution mode name

c034cee

juanpedromoreno requested changes May 22, 2020

View reviewed changes

Use http4s instead of hammock

b76e3bb

ememmr marked this pull request as ready for review May 26, 2020 08:01

ememmr requested a review from juanpedromoreno May 26, 2020 08:01

juanpedromoreno reviewed May 26, 2020

View reviewed changes

Add suggestions

ad18007

cb372 reviewed May 27, 2020

View reviewed changes

Use a single connection url for compendium

1ee90f8

juanpedromoreno reviewed May 28, 2020

View reviewed changes

Update default value, drop unused library version.

8a5fa4e

juanpedromoreno approved these changes May 28, 2020

View reviewed changes

cb372 reviewed May 28, 2020

View reviewed changes

Revert https changes.

9b2aa34

juanpedromoreno reviewed May 28, 2020

View reviewed changes

fedefernandez approved these changes May 28, 2020

View reviewed changes

Change raising error

41cf29e

ememmr merged commit cfac9f3 into master May 28, 2020

ememmr deleted the merge-plugins branch May 28, 2020 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge plugins #43

Merge plugins #43

ememmr commented May 20, 2020 •

edited

Loading

BenFradet left a comment

juanpedromoreno May 22, 2020

ememmr May 22, 2020

juanpedromoreno May 22, 2020 •

edited

Loading

ememmr May 26, 2020

juanpedromoreno May 26, 2020

ememmr May 27, 2020

juanpedromoreno May 26, 2020

fedefernandez May 26, 2020

cb372 May 27, 2020

ememmr May 27, 2020

cb372 May 27, 2020

juanpedromoreno May 26, 2020

ememmr May 27, 2020

cb372 May 27, 2020

juanpedromoreno May 26, 2020

juanpedromoreno May 26, 2020

juanpedromoreno May 26, 2020

juanpedromoreno May 26, 2020

cb372 May 27, 2020

juanpedromoreno May 28, 2020

juanpedromoreno May 28, 2020

cb372 May 28, 2020

ememmr May 28, 2020

juanpedromoreno May 28, 2020

juanpedromoreno May 28, 2020

fedefernandez May 28, 2020

fedefernandez May 28, 2020

fedefernandez May 28, 2020


		lazy val muSrcGenCompendiumServerPort: SettingKey[Int] =
		settingKey[Int]("Port of the compendium server")

	case class CompendiumMode[F[_]: Async: ConcurrentEffect](
	final case class CompendiumMode[F[_]: ConcurrentEffect](

	import cats.effect.{Async, ConcurrentEffect, Resource, Sync}
	import cats.effect.{Resource, ConcurrentEffect}

	Async[F].raiseError[File](
	ConcurrentEffect[F].raiseError[File](

	settingKey[String]("Url of the compendium server. By default, `http://localhost:47047`.")
	settingKey[String]("Url of the compendium server. By default, `https://localhost:47047`.")

	muSrcGenCompendiumServerUrl := "https://localhost:47047"
	muSrcGenCompendiumServerUrl := "http://localhost:47047"

Merge plugins #43

Merge plugins #43

Conversation

ememmr commented May 20, 2020 • edited Loading

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

juanpedromoreno May 22, 2020 • edited Loading

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

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

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

ememmr commented May 20, 2020 •

edited

Loading

juanpedromoreno May 22, 2020 •

edited

Loading