Autowire factory methods extended support scala2 #186
Conversation
| @@ -22,4 +22,4 @@ require(theC.b != null) | |||
|
|
|||
| require(created.size == 2) | |||
| require(created.contains("a")) | |||
| require(created.contains("B")) | |||
| require(created.contains("b")) | |||
There was a problem hiding this comment.
Probably not very relevant, but to test the ordering that we care about - that is, the ordering of flatMaps, not of method calls - in the factory methods that return resources, I think it would be better to populate the created collection as part of resource allocation/cleanup, e.g. Resource.make(IO { created.add("b"); B() })(_ => IO.unit)
|
|
||
| import cats.data.Op | ||
| import cats.effect.kernel | ||
| //hmmm lazy resource? |
|
A test case that we might want to add: that the resource values are re-used (also in a resource-factory setting). Sth along the lines of: case class A
case class C(b: B)
case class D(b: B)
case class E(c: C, d: D)
def makeB(a: A): Resource[B] = ...
val e = autowire[E](makeB _)here, |
| case (resource, acc) => | ||
| q"${resource.value}.flatMap((${resource.ident}: ${resource.tpe}) => $acc)" | ||
| } | ||
| val code = resources.foldRight(q"cats.effect.Resource.pure[cats.effect.IO, $targetType](com.softwaremill.macwire.autowire[$targetType](..${values.map(_.ident)}))") { |
There was a problem hiding this comment.
hm will this work if the factory method has both a from-resource, and an auto-wire parameter? e.g.
case class A()
class B
class C
val resourceB: Resource[B] = ...
def makeC(a: A, b: B): Resource[C] = ...
autowire[C](resourceB, makeC _)There was a problem hiding this comment.
maybe I'm missing some crucial detail of the algorithm, it might be useful to add a comment somewhere with rough steps which would explain what's happening.
There was a problem hiding this comment.
Yep, good catch, thanks! In that version it was not supported, but the fix was actually pretty straightforward
| .map(expr => FactoryMethod(expr.tree)) | ||
| .map(i => (i.`type`, i)) | ||
| .toMap | ||
| providers.foldLeft(init) { |
There was a problem hiding this comment.
I'm not quite sure yet what's happening here, but I have a feeling we should do some sorting here. If there are multiple non-value providers (e.g. factories), one factory producing the input for another, the sort order should reflect this.
Another test case to check ;-)
case class A()
class B
class C
def makeC(b: B): Resource[C] = ...
def makeB(a: A): Resource[B] = ...
autowire[C](makeC _, makeB _)There was a problem hiding this comment.
My idea was to keep the order of input dependencies. This kind of sorting may be useful, but when it comes to resource composition I believe that users may want to control the order.
There was a problem hiding this comment.
Hm I wouldn't say so, it's easy with two resources, but with more resources w/ dependencies it gets complex. You'd have to write this down yourself and perform the sorting. Perfect task for a computer ;)
There was a problem hiding this comment.
That said, it's a good idea to make the sorting stable, that is maintain the order given by the user if possible, but if dependencies require - reorder as needed
There was a problem hiding this comment.
Hmm, I see your point, but on the other hand it may be a good idea to make it configurable in future.
There was a problem hiding this comment.
I'd say that when using a DI library (or framework) one thing I don't want to be concerned with is order of initialization. I'd expect the library to handle this.
And if there's a specific ordering that is required - just make it explicit in code, by adding a dependency (e.g. via a factory method parameter)
mbore
force-pushed
the
autowire-factory-methods-extended-support-scala2
branch
from
ca2c086 to
21639ff
Compare
mbore
force-pushed
the
autowire-factory-methods-extended-support-scala2
branch
from
c11e14e to
91a74a9
Compare
| @@ -6,8 +6,9 @@ import cats.effect._ | |||
| val created = scala.collection.mutable.Set[String]() | |||
|
|
|||
| object Test { | |||
| val theB: B = B() | |||
There was a problem hiding this comment.
doesn't this change the test? Maybe we should have both :)
| case v => { | ||
| log(s"Resolving for FMs [${fms.mkString(", ")}] with values [${values.mkString(", ")}]") | ||
| //FIXME we need to make use of empty constructors at this point :/ | ||
| val (remainingFms, appliedFms) = v.partitionBifold(f => f.maybeResult(t => findProviderIn(values)(t).orElse(freshInstanceFromEmptyConstructor(t))).toRight(f)) |
There was a problem hiding this comment.
This is a very brute-force sorting algorithm ;) Aside from performance (n^2) and the (fixable?) need for no-arg constructors, there's another flaw that's directly impacting user experience - we can't let the users know which dependency is missing, and what's the path.
That's why I'd still insist on creating a proper dependency graph (vertices - providers, edges - dependencies) and sorting the vertices topologically, which would give us the proper creation order, plus a nice way to report errors.
No description provided.