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

Strange usage of ExecutorEffectsInterpreter in ExecutorEffectHandler #15

Open
adolgiy opened this issue Nov 9, 2021 · 2 comments
Open

Comments

@adolgiy
Copy link

adolgiy commented Nov 9, 2021

Current implementation in 'master' looks like:

typealias ExecutorEffectsInterpreter<Eff, Msg> = ExecutorService.(eff: Eff, listener: (Msg) -> Unit) -> Unit

class ExecutorEffectHandler<Msg : Any, Eff : Any>(
    // ...
) : EffectHandler<Eff, Msg> {

    override fun handleEffect(eff: Eff) {
        effectsExecutorService.run { effectsInterpreter(eff, listener ?: {}) } // !!! THIS
    }
}

Line with comment do nothing except proxying call to extension function via run {} call. Shouldn't it be fixed like this?

effectsExecutorService.execute { effectsInterpreter(eff, listener ?: {}) }
@Mishkun
Copy link
Owner

Mishkun commented Nov 10, 2021

EffectsInterpreter is an extension function to ExecutorService, so yeah it is better to rewrite this as

effectsInterpreter(effectsExecutorService, eff, listener ?: {})

@adolgiy
Copy link
Author

adolgiy commented Nov 10, 2021

But in this way it will not be scheduled on given ExecutorService. Shouldn't ExecutorEffectHandler handle all effects on this executor? If should, why would you pass ExecutorService as param?

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

No branches or pull requests

2 participants