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

Increase code coverage (pt2). #389

Merged
merged 1 commit into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 1 addition & 13 deletions formula-android/src/main/java/com/instacart/formula/Renderer.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,12 @@ package com.instacart.formula
* renderText("three")
* ```
*/
class Renderer<in RenderModel> private constructor(
class Renderer<in RenderModel>(
private val renderFunction: (RenderModel) -> Unit
) : (RenderModel) -> Unit {

companion object {

/**
* Creates a render function that does nothing.
*/
fun <T> empty() = create<T> { }

/**
* Creates a render function.
*/
operator fun <RenderModel> invoke(render: (RenderModel) -> Unit): Renderer<RenderModel> {
return Renderer(renderFunction = render)
}

/**
* Creates a render function.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ package com.instacart.formula.test

import com.instacart.formula.IFormula
import com.instacart.formula.RuntimeConfig
import com.instacart.formula.plugin.Dispatcher
import com.instacart.formula.plugin.Inspector
import com.instacart.formula.rxjava3.toObservable
import io.reactivex.rxjava3.subjects.BehaviorSubject

Expand All @@ -12,16 +10,8 @@ import io.reactivex.rxjava3.subjects.BehaviorSubject
*/
class RxJavaFormulaTestDelegate<Input : Any, Output : Any, FormulaT : IFormula<Input, Output>>(
override val formula: FormulaT,
isValidationEnabled: Boolean = true,
inspector: Inspector?,
dispatcher: Dispatcher?,
runtimeConfig: RuntimeConfig,
) : FormulaTestDelegate<Input, Output, FormulaT> {
private val runtimeConfig = RuntimeConfig(
isValidationEnabled = isValidationEnabled,
inspector = inspector,
defaultDispatcher = dispatcher,
)

private val inputRelay = BehaviorSubject.create<Input>()
private val observer = formula
.toObservable(inputRelay, runtimeConfig)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.instacart.formula.test

import com.instacart.formula.Action
import com.instacart.formula.IFormula
import com.instacart.formula.RuntimeConfig
import com.instacart.formula.plugin.Dispatcher
import com.instacart.formula.plugin.Inspector

Expand All @@ -15,7 +16,13 @@ fun <Input : Any, Output : Any, F: IFormula<Input, Output>> F.test(
inspector: Inspector? = null,
dispatcher: Dispatcher? = null,
): TestFormulaObserver<Input, Output, F> {
val delegate = RxJavaFormulaTestDelegate(this, isValidationEnabled, inspector, dispatcher)
val runtimeConfig = RuntimeConfig(
isValidationEnabled = isValidationEnabled,
inspector = inspector,
defaultDispatcher = dispatcher,
)

val delegate = RxJavaFormulaTestDelegate(this, runtimeConfig)
return TestFormulaObserver(delegate)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package com.instacart.formula.internal
import com.instacart.formula.FormulaPlugins
import com.instacart.formula.IFormula
import com.instacart.formula.plugin.Inspector
import java.lang.IllegalStateException

/**
* Keeps track of child formula managers.
Expand All @@ -12,7 +11,7 @@ internal class ChildrenManager(
private val delegate: FormulaManagerImpl<*, *, *>,
private val inspector: Inspector?,
) {
private var children: SingleRequestMap<Any, FormulaManager<*, *>>? = null
private val children: SingleRequestMap<Any, FormulaManager<*, *>> = LinkedHashMap()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to optimize children map initialization as FormulaManager optimizes this already

private var indexes: MutableMap<Any, Int>? = null
private var pendingRemoval: MutableList<FormulaManager<*, *>>? = null

Expand All @@ -26,7 +25,7 @@ internal class ChildrenManager(
fun prepareForPostEvaluation() {
indexes?.clear()

children?.clearUnrequested(this::prepareForTermination)
children.clearUnrequested(this::prepareForTermination)
}

fun terminateChildren(evaluationId: Long): Boolean {
Expand All @@ -42,20 +41,20 @@ internal class ChildrenManager(
}

fun markAsTerminated() {
children?.forEachValue { it.markAsTerminated() }
children.forEachValue { it.markAsTerminated() }
}

fun performTerminationSideEffects(executeTransitionQueue: Boolean) {
children?.forEachValue { it.performTerminationSideEffects(executeTransitionQueue) }
children.forEachValue { it.performTerminationSideEffects(executeTransitionQueue) }
}

fun <ChildInput, ChildOutput> findOrInitChild(
key: Any,
formula: IFormula<ChildInput, ChildOutput>,
input: ChildInput,
): FormulaManager<ChildInput, ChildOutput> {
val childHolder = childFormulaHolder(key, formula, input)
return if (childHolder.requested) {
val childManagerEntry = getOrInitChildManager(key, formula, input)
return if (childManagerEntry.requested) {
val logs = duplicateKeyLogs ?: run {
val newSet = mutableSetOf<Any>()
duplicateKeyLogs = newSet
Expand All @@ -76,38 +75,27 @@ internal class ChildrenManager(
)
}

if (key is IndexedKey) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removing this as this will never happen

// This should never happen, but added as safety
throw IllegalStateException("Key already indexed (and still duplicate).")
}

val index = nextIndex(key)
val indexedKey = IndexedKey(key, index)
findOrInitChild(indexedKey, formula, input)
} else {
childHolder.requestAccess {
"There already is a child with same key: $key. Override [Formula.key] function."
}
childManagerEntry.requested = true
childManagerEntry.value
}
}

private fun prepareForTermination(it: FormulaManager<*, *>) {
pendingRemoval = pendingRemoval ?: mutableListOf()
val list = pendingRemoval ?: mutableListOf()
pendingRemoval = list
it.markAsTerminated()
pendingRemoval?.add(it)
list.add(it)
}

private fun <ChildInput, ChildOutput> childFormulaHolder(
private fun <ChildInput, ChildOutput> getOrInitChildManager(
key: Any,
formula: IFormula<ChildInput, ChildOutput>,
input: ChildInput,
): SingleRequestHolder<FormulaManager<ChildInput, ChildOutput>> {
val children = children ?: run {
val initialized: SingleRequestMap<Any, FormulaManager<*, *>> = LinkedHashMap()
this.children = initialized
initialized
}

val childFormulaHolder = children.findOrInit(key) {
val implementation = formula.implementation
FormulaManagerImpl(
Expand Down Expand Up @@ -136,7 +124,12 @@ internal class ChildrenManager(
initialized
}

val index = indexes.getOrElse(key) { 0 } + 1
val previousIndex = indexes[key]
val index = if (previousIndex == null) {
0
} else {
previousIndex + 1
}
indexes[key] = index
return index
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import com.instacart.formula.plugin.Dispatcher
* Note: this class is not a data class because equality is based on instance and not [key].
*/
@PublishedApi
internal class ListenerImpl<Input, State, EventT>(val key: Any) : Listener<EventT> {
internal class ListenerImpl<Input, State, EventT>(private val key: Any) : Listener<EventT> {

@Volatile private var manager: FormulaManagerImpl<Input, State, *>? = null
@Volatile private var snapshotImpl: SnapshotImpl<Input, State>? = null
Expand All @@ -21,11 +21,11 @@ internal class ListenerImpl<Input, State, EventT>(val key: Any) : Listener<Event
val manager = manager ?: return

when (val type = executionType) {
is Transition.Batched -> handleBatched(type, event)
Transition.Immediate -> executeWithDispatcher(Dispatcher.None, event)
Transition.Background -> executeWithDispatcher(Dispatcher.Background, event)
is Transition.Batched -> handleBatched(manager, type, event)
Transition.Immediate -> executeWithDispatcher(manager, Dispatcher.None, event)
Transition.Background -> executeWithDispatcher(manager, Dispatcher.Background, event)
// If transition does not specify dispatcher, we use the default one.
else -> executeWithDispatcher(manager.defaultDispatcher, event)
else -> executeWithDispatcher(manager, manager.defaultDispatcher, event)
}
}

Expand All @@ -50,17 +50,22 @@ internal class ListenerImpl<Input, State, EventT>(val key: Any) : Listener<Event
snapshotImpl?.dispatch(transition, event)
}

private fun handleBatched(type: Transition.Batched, event: EventT) {
val manager = manager ?: return

private fun handleBatched(
manager: FormulaManagerImpl<Input, State, *>,
type: Transition.Batched,
event: EventT,
) {
manager.batchManager.add(type, event) {
val deferredTransition = DeferredTransition(this, event)
manager.onPendingTransition(deferredTransition)
}
}

private fun executeWithDispatcher(dispatcher: Dispatcher, event: EventT) {
val manager = manager ?: return
private fun executeWithDispatcher(
manager: FormulaManagerImpl<Input, State, *>,
dispatcher: Dispatcher,
event: EventT,
) {
manager.queue.postUpdate(dispatcher) {
val deferredTransition = DeferredTransition(this, event)
manager.onPendingTransition(deferredTransition)
Expand Down
28 changes: 11 additions & 17 deletions formula/src/main/java/com/instacart/formula/internal/Listeners.kt
Original file line number Diff line number Diff line change
@@ -1,31 +1,20 @@
package com.instacart.formula.internal

import java.lang.IllegalStateException

internal class Listeners {
private var listeners: SingleRequestMap<Any, ListenerImpl<*, *, *>>? = null
private var indexes: MutableMap<Any, Int>? = null

private fun duplicateKeyErrorMessage(key: Any): String {
return "Listener $key is already defined. Unexpected issue."
}

fun <Input, State, Event> initOrFindListener(key: Any, useIndex: Boolean): ListenerImpl<Input, State, Event> {
val currentHolder = listenerHolder<Input, State, Event>(key)
return if (currentHolder.requested && useIndex) {
if (key is IndexedKey) {
Copy link
Collaborator Author

@Laimiux Laimiux Sep 12, 2024

Choose a reason for hiding this comment

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

This won't happen so no need to check

// This should never happen, but added as safety
throw IllegalStateException("Key already indexed (and still duplicate).")
}

return if (!currentHolder.requested) {
currentHolder.requested = true
currentHolder.value as ListenerImpl<Input, State, Event>
} else if (useIndex) {
val index = nextIndex(key)
val indexedKey = IndexedKey(key, index)
initOrFindListener(indexedKey, useIndex)
} else {
currentHolder
.requestAccess {
duplicateKeyErrorMessage(currentHolder.value.key)
} as ListenerImpl<Input, State, Event>
throw IllegalStateException("Listener $key is already defined. Unexpected issue.")
}
}

Expand Down Expand Up @@ -61,7 +50,12 @@ internal class Listeners {
initialized
}

val index = indexes.getOrElse(key) { 0 } + 1
val previousIndex = indexes[key]
val index = if (previousIndex == null) {
0
} else {
previousIndex + 1
}
indexes[key] = index
return index
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,6 @@ package com.instacart.formula.internal
internal class SingleRequestHolder<T>(val value: T) {
var requested: Boolean = false

inline fun requestAccess(errorMessage: () -> String): T {
if (requested) {
throw IllegalStateException(errorMessage())
}

requested = true
return value
}

fun reset() {
requested = false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import com.instacart.formula.TransitionContext
import java.lang.IllegalStateException
import kotlin.reflect.KClass

internal class SnapshotImpl<out Input, State> internal constructor(
internal class SnapshotImpl<out Input, State>(
override val input: Input,
override val state: State,
listeners: Listeners,
Expand Down
Loading
Loading