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

Support default values in constructors and methods in Kotlin #77

Closed
sergejsha opened this issue Mar 9, 2019 · 5 comments · Fixed by #86
Closed

Support default values in constructors and methods in Kotlin #77

sergejsha opened this issue Mar 9, 2019 · 5 comments · Fixed by #86
Labels
enhancement New feature or request
Milestone

Comments

@sergejsha
Copy link
Owner

Magnet ignores default values while injecting parameters into constructors and methods in Kotlin.
Expected behavior: Magnet does not inject values for parameters with defaults.

@sergejsha sergejsha added this to the 3.1 milestone Mar 9, 2019
@sergejsha sergejsha changed the title Support default values in constructors and methods for Kotlin Support default values in constructors and methods in Kotlin Mar 9, 2019
@realdadfish
Copy link
Contributor

realdadfish commented Mar 9, 2019

What is the rule in Magnet anyways here? Imagine I have provided A and B, which one would be the constructor Magnet would pick?

@Instance(type = Foo::class)
class Foo(private val a: A, private val b: B) {
     constructor(a: A) : this(a, B())
}

IIRC @Instance is only allowed on static methods and types, not constructors, so I have no way to pick a specific constructor Magnet should use for construction, right?

@sergejsha
Copy link
Owner Author

sergejsha commented Mar 9, 2019

What is the rule in Magnet anyways here? Imagine I have provided A and B, which one would be the constructor Magnet would pick?

IIRC @Instance is only allowed on static methods and types, not constructors, so I have no way to pick a specific constructor Magnet should use for construction, right?

This is correct. Magnet will fail to compile because it expects a single non-private constructor. This was a deliberate design decision. The reason for it is simpler annotation syntax (also for Kotlin) and enforced single-constructor design, which is, in my opinion, cleaner than multi-constructor one, and Kotlin constructors kind of confirming that.

This ticket is rather about default parameters in constructor. Let's take the example down below.

@Instance(type = GalleryScannerService::class)
internal class DefaultGalleryScannerService(
    mediaStoragePermissions: MediaStoragePermissions,
    mediaStorageObserver: MediaStorageObserver,
    mediaStorageSyncer: MediaStorageSyncer,
    computationScheduler: Scheduler = Schedulers.computation(),
    ioScheduler: Scheduler = Schedulers.io()
) : GalleryScannerService

It declares default values for computationScheduler and ioScheduler parameters. Current Magnet version overwrites those values with the ones from the scope, if they are available there. This ticket is about to respect given Kotlin defaults and keep them intact.

You might have a question why those two are in constructor at all. They are there for easier testing. There is no need to apply any special rules in tests. It's enough to provide TestScheduler or Schedulers.trampoline() to the instance and to test what ever needed, including debouncing and timeouts.

@realdadfish
Copy link
Contributor

Ok, I see. I grouped together the various schedulers and more into a SchedulerProvider, of which I have a test implementation, so it is not much of a hassle for me to provide one instance of that provider in the root scope to have it available everywhere. I understand however if you inject individual instances of Scheduler you'd have to manually @Classifier them for injection, so it is easier to just use the static instance methods to return default values.

@sergejsha
Copy link
Owner Author

After more detailed analysis of the problem I realized that the constructor which sets default values in Kotlin is implemented as a synthetic method with Kotlin-specific parameters. Thus calling it properly from Java is practically impossible. The only option to respect default values in constructor would be to generate Kotlin code instead of Java code, which goes beyond the scope of this ticket.

@sergejsha sergejsha added the wontfix This will not be worked on label Mar 23, 2019
@sergejsha sergejsha reopened this Apr 20, 2019
@stale stale bot removed the wontfix This will not be worked on label Apr 20, 2019
@sergejsha
Copy link
Owner Author

sergejsha commented Apr 20, 2019

Kotlin supports generation of overloaded Java constructors for a kotlin constructor with default arguments when @JvmOverloads constructor syntax is used.

@Instance(type = TemplateObserver::class)
internal class DefaultTemplateObserver @JvmOverloads constructor(
    private val database: Database,
    private val marshaller: Marshaller,
    private val templateDao: TemplateDao,
    ioScheduler: Scheduler = Schedulers.io()
) : TemplateObserver { ... }

Because those constructors can be called from Java, Magnet will also be able to call them from generated factories.

@sergejsha sergejsha modified the milestones: 3.1, 3.2 Apr 21, 2019
@sergejsha sergejsha added the enhancement New feature or request label Apr 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants