Autowire refactor #188
Autowire refactor #188
Conversation
|
The formatting looks weird - e.g. in |
|
|
||
| val graph = new CatsProvidersGraphContext[c.type](c, log) | ||
| val sortedProviders = graph.buildGraph(dependencies.toList).topologicalOrder() |
mbore
force-pushed
the
autowire-refactor
branch
from
77580d1 to
37c26d6
Compare
|
|
||
| val typeCheckUtil: TypeCheckUtil[c.type] | ||
|
|
||
| trait Provider { |
There was a problem hiding this comment.
Yes, It could be. I left it as a non-sealed trait because refactor required to introduce pure autowire is going to require adding new classes extending Provider trait. But for now, just to focus on the current use case, let's make it sealed.
|
|
||
| } | ||
|
|
||
| case class Creator( |
There was a problem hiding this comment.
what's the difference between a creator and a factory? isn't a constructor/apply method just a special case of factory?
There was a problem hiding this comment.
Ah there is no real difference in terms of the dependencies graph. I was focused on the different sources of these providers and missed that they are, actually the same... Thanks for the comment!
| class B() extends T | ||
|
|
||
| object Test { | ||
| val theA = autowire[A](new B()) |
There was a problem hiding this comment.
why is this ambiguous? B is the only instance of type T that we have?
There was a problem hiding this comment.
| @@ -3,6 +3,7 @@ package com.softwaremill.macwire.autocats | |||
| import scala.reflect.macros.blackbox | |||
| import cats.effect.{IO, Resource => CatsResource} | |||
| import com.softwaremill.macwire.internals._ | |||
| import com.softwaremill.macwire.autocats.internals._ | |||
|
|
|||
| object MacwireCatsEffectMacros { | |||
There was a problem hiding this comment.
[minor] name doesn't match file name :)
|
|
||
| log(s"Input providers order [${inputProvidersOrder.mkString(", ")}]") | ||
|
|
||
| new CatsProvidersGraph(inputProvidersOrder ++ resolvedCtx.providers.diff(inputProvidersOrder), rootProvider) |
There was a problem hiding this comment.
I'm not sure what's the purpose here of the inputProvidersOrder and the other providers? Can't we just pass the providers providers ++ fms to the graph and sort them?
There was a problem hiding this comment.
This point is necessary to sort independent providers in the same way they were passed (I think it's a good initial approach to the problem, because it makes the result order predictable).
I was thinking about two other approaches to this problem:
- implicit order of providers that needs to be preserved during the whole resolution process - it would be definitely harder to maintain
- additional sorting logic in
topologicalSort- I think it would harmCatsProvidersGraphencaptulation
There was a problem hiding this comment.
I still don't understand why we iterate over rawProviders twice :)
There was a problem hiding this comment.
Ah did you ask about these providers and fms -https://github.com/softwaremill/macwire/pull/188/files/a78010f1dd0991bf326716caacf5ff14946fb3b6#diff-8923920dc067a35890d1e81851c52f2387bb9d3e173c036c9a714372d7146558R61 ?
The fms name is actually incorrect, because it's a list of FactoryMethodTree that needs to be resolved in resolveFactoryMethods method. We cannot use providers from BuilderContext directly, because resolveFactoryMethods does not preserve the input order, so I think we cannot avoid the second iteration in a simple way.
I extracted a common part to a variable.
|
Don't we need to update something in the readme? Otherwise I think we're good to merge & release :) |
Changes required in #186 led me to rework of
autowire, therefor I decided to create a separate PR