Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement wireResource for scala 2 #175

Merged
merged 32 commits into from
Oct 21, 2021
Merged

Conversation

mbore
Copy link
Contributor

@mbore mbore commented Aug 27, 2021

No description provided.

@mbore mbore mentioned this pull request Aug 27, 2021
@adamw
Copy link
Member

adamw commented Oct 11, 2021

One more naming idea: wireAuto in the package macwire.auto.cats?

@mbore
Copy link
Contributor Author

mbore commented Oct 11, 2021

@adamw actually in this case semiauto is more appropriate since we need to pass a list of available instances. Regarding the method name, I think that wireApp may be a good idea, because I'd like to make it work for instances wrapped in effects (e.g. IO[DatabaseAccess]) in future.

@adamw
Copy link
Member

adamw commented Oct 11, 2021

Hmm well but wireApp suggests that it's only for top-level applications, which is not true.

Another candidate: autowire, which is almost Spring ;) But you're right that semiAutoWire would be more correct, however it's longer and looks uglier in code ;)

@adamw
Copy link
Member

adamw commented Oct 11, 2021

And yes, sure, we do want to support instances wrapped in effects :)

@adamw
Copy link
Member

adamw commented Oct 11, 2021

Looks good :) There's a lot of tests to add, though. Maybe I'll just add them and then we'll have sth like TDD ;)

One thing that we'll need to decide is how to re-use instances. Right now we are re-using anything that is created by a resource - and this makes sense. When creating an instance of a class, do we want to re-use it? That is, should the following (assuming not only Resources are implemented):

case class A
case class B(a1: A, a2: A)

val b = wireX[B]

generate either (1):

val b = B(A(), A())

or (2):

val b = {
  val x = A()
  B(x, x)
}

Here it doesn't make a difference, but maybe for some cases it does make a difference whether a dependency is a singleton and re-used, or not.

Probably (1) is the better default, as it follows other DI frameworks where "dependent" scope is the default. And we could achieve singleton by explicitly passing in the instance. So I guess that's solved, or do you have another opinion? :)

@mbore mbore marked this pull request as ready for review October 11, 2021 20:20
@mbore mbore force-pushed the simplified-wireResoure-scala2 branch from 6b43819 to ee50b16 Compare October 12, 2021 19:59
@mbore mbore force-pushed the simplified-wireResoure-scala2 branch from ee50b16 to 42b5eea Compare October 12, 2021 20:10
build.sbt Outdated
@@ -167,6 +171,21 @@ lazy val macrosAkkaTests = projectMatrix
.dependsOn(macrosAkka, testUtil)
.jvmPlatform(scalaVersions = scala2)

lazy val macrosCatsEffect = projectMatrix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rename this to macrosAutoCats, and similarly, the package to .auto.cats. The current package name is way too long ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about .auto.catssupport? .auto.cats clashes with the cats package

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm it conflicts, how? I guess import macwire.auto.cats._ or import macwire.auto.cats.autowire should work just fine? I've got a tapir.integ.cats package and as far as I know it works just fine ;)

We can come up with a different name, though ... however catssupport looks cumbersome. Maybe .autocats? ;)

!name.startsWith("java.lang.") && !name.startsWith("scala.")
}

def findeProvider(tpe: Type): Option[Tree] = findInstance(tpe)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[typo] -> findProvider

README.md Outdated
In case you need to build an instance from some particular instances and factory methods it's recommended to use `autowire`. This feature is intended to interpolate with fp libraries (currently we support `cats`).

`autowire` takes as an argument a list which may contain:
* instances (e.g. `new A()`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think values would be more correct. Any value will do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact ... do we have tests, that you can use a sub-type, where the dependency is of a super-type? E.g. I declare that I need a dependency of type trait A, but I provide a value of type class AA extends A. Same for resources etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now we don't have tests for this case and we do not currently support it. I will add it to todo list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's keep the todo as issues :) I've created #183 for this

* factory methods (e.g. `C.create _`)
* cats.effect.Resource (e.g. `cats.effect.Resource[IO].pure(new A())`)
* cats.effect.IO (e.g. `cats.effect.IO.pure(new A())`)
Based on the given list it creates a set of available instances and performs `wireRec` bypassing the instances search phase. The result of the wiring is always wrapped in `cats.effect.Resource`. For example:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if multiple values for a given type are found? E.g. through a value and a resource? I think it should be an error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it searches for instances, then resources, then effects. I agree that is should fail in such case, also it may be worth to check if all passed instances/resources/factory methods are used in the generated code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created #181 & #182 to cover these


val theDatabaseAccess: Resource[IO, DatabaseAccess] = Resource.pure(new DatabaseAccess())

val theUserStatusReader: Resource[IO, UserStatusReader] = autowire[UserStatusReader](theDatabaseAccess)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ this looks nice :)

@mbore
Copy link
Contributor Author

mbore commented Oct 15, 2021

Actually I'm still not sure if we want to support no parameters constructors.
It may cause difficult to debug mistakes like "forgot to pass filled config", for example:

import cats.effect._
import cats.effect.unsafe.implicits.global

class MutableConfig() {
  var port: Option[Int] = None
  var host: Option[String] = None
}

class Service(cfg: MutableConfig) {
  println(s"[${cfg.host}]:[${cfg.port}]")
}

object Main extends App {

  def loadConfig(): Resource[IO, MutableConfig] = Resource.pure {
    val mc = new MutableConfig()
    mc.host = Some("xyz")
    mc.port = Some(8080)

    mc
  }

  val cfg = loadConfig()

  val service = autowire[Service]()

  service.allocated.unsafeRunSync()._1

}

it works and prints [None]:[None].
I see that it may reduce boilerplate in some cases, but I'm not sure if it's worth to risk.

@adamw
Copy link
Member

adamw commented Oct 17, 2021

@mbore I've moved the no-arg constructor problem to #184 as well

@mbore mbore mentioned this pull request Oct 18, 2021
@mbore mbore force-pushed the simplified-wireResoure-scala2 branch from 2bc32fb to 53cdea2 Compare October 21, 2021 16:15
@adamw adamw merged commit 5b0f8fc into master Oct 21, 2021
@adamw adamw deleted the simplified-wireResoure-scala2 branch September 10, 2024 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants