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

Use autowire #766

Merged
merged 10 commits into from
Feb 16, 2022
Merged

Use autowire #766

merged 10 commits into from
Feb 16, 2022

Conversation

mbore
Copy link
Contributor

@mbore mbore commented Dec 27, 2021

No description provided.

@adamw
Copy link
Member

adamw commented Jan 3, 2022

Seems we're not using autowire? ;)

@mbore mbore force-pushed the use-autowire branch 3 times, most recently from 3271ed7 to f90d94d Compare January 10, 2022 20:10
@mbore mbore marked this pull request as ready for review January 10, 2022 20:28
import io.prometheus.client.CollectorRegistry
import sttp.client3.SttpBackend

object DependenciesFactory {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid a single class where the dependencies are defined. This won't scale. Let's find a way to dismantle this :)


def buildPasswordResetAuthToken(passwordResetCodeModel: PasswordResetCodeModel): PasswordResetAuthToken = new PasswordResetAuthToken(passwordResetCodeModel)

def buildEmailSender(sttpBackend: SttpBackend[IO, Any], config: EmailConfig): EmailSender = if (config.mailgun.enabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this can be moved to EmailSenders companion object, for example?


def buildApiKeyAuthToken(apiKeyModel: ApiKeyModel): ApiKeyAuthToken = new ApiKeyAuthToken(apiKeyModel)

def buildPasswordResetAuthToken(passwordResetCodeModel: PasswordResetCodeModel): PasswordResetAuthToken = new PasswordResetAuthToken(passwordResetCodeModel)
Copy link
Member

Choose a reason for hiding this comment

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

seems like constructor invocation, can't we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the created instance as AuthTokenOps[PasswordResetCode], so for now unfortunately we can't, but once we implement this one, we will be able to use it here instead.

DummyEmailSender
}

def buildEmailScheduler(emailModel: EmailModel, idGenerator: IdGenerator, emailSender: EmailSender, config: EmailConfig, xa: Transactor[IO]) =
Copy link
Member

Choose a reason for hiding this comment

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

same here - just constructor invocation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, but we need useImplementation feature to get rid of it :(


lazy val xa = new DB(config.db).transactorResource

val mainTask = DependenciesFactory.resource(
Copy link
Member

Choose a reason for hiding this comment

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

ideally the autowire invocation should be here

build.sbt Outdated
@@ -80,6 +81,11 @@ val emailDependencies = Seq(
"com.sun.mail" % "javax.mail" % "1.6.2" exclude ("javax.activation", "activation")
)

val macwireDependencies = Seq(
"com.softwaremill.macwire" %% "macros" % macwireVersion,
Copy link
Member

Choose a reason for hiding this comment

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

do we need this dependency at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah right, we don't

@adamw
Copy link
Member

adamw commented Jan 14, 2022

Can you create a PR against master, as monix-removal is already merged?

@mbore mbore changed the base branch from monix-removal to master January 20, 2022 13:35
@@ -80,6 +81,10 @@ val emailDependencies = Seq(
"com.sun.mail" % "javax.mail" % "1.6.2" exclude ("javax.activation", "activation")
)

val macwireDependencies = Seq(
"com.softwaremill.macwire" %% "macrosautocats" % macwireVersion
Copy link
Member

Choose a reason for hiding this comment

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

side-note for macwire - I think we should rename the module to sth readable, such as macros-auto-cats ;)

@adamw
Copy link
Member

adamw commented Jan 20, 2022

Let me know what you think @mbore and if it's ok we can merge :)

@adamw adamw merged commit 06807e8 into master Feb 16, 2022
@mergify mergify bot deleted the use-autowire branch February 16, 2022 19:45
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