Verizon / remotely

An elegant RPC system for reasonable people
http://verizon.github.io/remotely/
Apache License 2.0
306 stars 39 forks source link

core: GenClient & GenServer are broken due to restricted accessibility of Field constructor #89

Closed ahjohannessen closed 9 years ago

ahjohannessen commented 9 years ago
ahjohannessen commented 9 years ago

@timperrett @jedesah WDYT?

ahjohannessen commented 9 years ago

In fact, only @GenClient and @GenServer usage that works for 1.4.0 based projects are those that have the top-level package remotely.

ahjohannessen commented 9 years ago

In order to summerize a bit, the code that causes issues is located in Gen.scala and is used by GenClient and GenServer:

  def liftField(c: Context)(field: Field[Any]): c.universe.Tree = {
    import c.universe._
    q"_root_.remotely.Field(${field.name}, ${field.typeString})"
  }

Even if Gen is in package remotely and the problematic constructor of Field is private[remotely] we get an error stating that:

constructor Field in class Field cannot be accessed in object Client

Why that is confuses me a bit, but my PR solves the issue at hand. I can understand why that constructor is meant to be private, but as of now 1.4.0 is not usable, and I think that this is a reasonable fix for the time being despite it allows people to shoot themselves in the foot.

Alternatively:

case class Field[+A] private[remotely](name: String, typeString: String)
object Field {
  def loose[A](name: String, typeString: String) = Field[A](name, typeString)
  def strict[A:TypeTag](name: String) = Field[A](name, Remote.toTag[A])
}

and adjust liftField to do this:

q"_root_.remotely.Field.loose[Any](${field.name}, ${field.typeString})"

in order to signal that is irregular usage by way of explicitly using loose, but honestly I don't know how much that is good for, especially, if the API grows such that the user is not directly exposed to Field normally.

jedesah commented 9 years ago

@ahjohannessen I agree with you that this is a very simple change that solves a major problem so we should do it now and consider if there is a better way later. Can you insert a comment above Field to remind us why it was made public?

ahjohannessen commented 9 years ago

Sure thing @jedesah :)

ahjohannessen commented 9 years ago

@jedesah I have updated it with this comment:

// TODO: ctor should really be private[remotely], but due
// to macro code generation issue in liftField (Gen.scala)
// we open it for the time being. See Github issue #89.
ahjohannessen commented 9 years ago

and consider if there is a better way later

Definitely important

ahjohannessen commented 9 years ago

@jedesah @timperrett If this gets merged and accepted, please publish a 1.4.1 soon :)

timperrett commented 9 years ago

@ahjohannessen here you go: https://bintray.com/oncue/releases/remotely/1.4.1/view

ahjohannessen commented 9 years ago

@jedesah @timperrett thanks guys :+1: