search

etiennelenhart / Eiffel

Redux-inspired Android architecture library leveraging Architecture Components and Kotlin Coroutines
MIT License
211 stars 14 forks source link

Fix: After pipe's Action type #107

Closed jordond closed 5 years ago

jordond commented 5 years ago

We are casting the action action as T, but the lambda signature was still set to the original A.

I also added a default value for after. Which has the benefit of being able to use pipe(before={...}) without having to supply an after lambda as well. This of course also allows the developer to just do pipe(), and nothing will happen...

I have a proposal PR coming, that might help with that.

etiennelenhart commented 5 years ago

The default value for after is definitely missing, but I think the (state: S, action: A?) -> Unit signature is correct, since the provided action is the result of applying all following interceptions. So it can be of any subtype of A and is nullable. The cast action as T for after seems to be the actual error. Classic copy-paste mistake… 😓

jordond commented 5 years ago

Ah, that was my first thought as well. But then I rationalized it as the dev might want to pipe after some other Action (in the before) was adapted to the target Action.

Does that make sense? Hah in my head it does.

But I can definitely make the change if you want, as that was my original plan.

etiennelenhart commented 5 years ago

Oh, you're absolutely correct. 😅 With the if (target(action)) check it's already working like that… Guess I mixed that up as well. Then your change is completely valid. Although you can even make it a non-nullable type then. (Change it to T instead of T?.) Thanks for the catch!