-
Notifications
You must be signed in to change notification settings - Fork 151
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
Use autowire #766
Conversation
Seems we're not using autowire? ;) |
3271ed7
to
f90d94d
Compare
import io.prometheus.client.CollectorRegistry | ||
import sttp.client3.SttpBackend | ||
|
||
object DependenciesFactory { |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 EmailSender
s companion object, for example?
|
||
def buildApiKeyAuthToken(apiKeyModel: ApiKeyModel): ApiKeyAuthToken = new ApiKeyAuthToken(apiKeyModel) | ||
|
||
def buildPasswordResetAuthToken(passwordResetCodeModel: PasswordResetCodeModel): PasswordResetAuthToken = new PasswordResetAuthToken(passwordResetCodeModel) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]) = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Can you create a PR against |
@@ -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 |
There was a problem hiding this comment.
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
;)
Let me know what you think @mbore and if it's ok we can merge :) |
No description provided.