search

vlingo / xoom-common

The VLINGO XOOM platform SDK common tools shared across various projects.
Other
16 stars 10 forks source link

Completes `andThen` allows different return types #7

Closed ghost closed 5 years ago

ghost commented 5 years ago

Continuation of vlingo-actors/pull/19.

Most of tests that fail is because it expects to have the reference to the previous BasicCompletes. Since the current andThen uses andThenInto as inner implementation (don't exposed to the end user), we lose that reference and state (to check completes.hasFailed()) to the previous BasicCompletes instance.

Should we pass the ActiveState to the next BasicCompletes? I would like to fix the tests instead of chaning the current behaviour.

VaughnVernon commented 5 years ago

@aleixmorgadas I love this! I noticed that some BasicCompletes<T> tests are failing on Travis. Those should be fixed before I merge. Thanks!

ghost commented 5 years ago

About Completes<T> & BasicCompletes<T>,

I've some questions/assumptions regarding the failing tests and a possible underlying breaking change before I continue.

andThen isn't a synced method under the hood anymore, so T Completes.outcome() returns the first BasicCompletes.outcome() instance result. outcome() should return the latest computed value.

A similar thing happens to methods like boolean Completes.hasFailed(), it returns the first ActiveState and it should return the latest computed state.

I'm wondering if Completes should have outcome() method or we should remove it. Since a Completes can be async and there's not a way to determine andThenInto has been called or not, outcome() might not be resolved yet. In any case, if we solve the issue returning the latest State, we solve the issue with the outcome() as well in case of sync operation. What it's not clear to me is what happens when there's an async operation and outcome() is called.

On the other hand, in case we keep outcome(), I think T Completes.outcome() should be Outcome<T> Completes.outcome().

VaughnVernon commented 5 years ago

@aleixmorgadas I guess I am a little confused because the other day we discussed andThen(...) and I thought you confirmed that it would continue as a synchronous method and that only andThenInto(...) would be async. In principle I don't think that we really absolutely need outcome() but I am certain that some locked in the imperative universe will want it. Also it's helpful/useful when testing.

The way to handle what you are suggesting is to use a LinkedList to enable navigation from any number of nested Completes<T> instances back to the root. I purposely avoided that however because we had discussed that andThen(...) would never need it and any eventual outcome of andThenInto(...) would finally be consumed or applied from an andThen(...) that followed the andThenInto(...). Plus, understanding that we wouldn't need it I wanted to avoid the more complex implementation and also avoid the memory overhead. (BasicCompletes<T> has already grown in required data far beyond my original vision, but maybe that's not really important.)

So I think that leaving the outcome() as is, as well as ensuring that andThen(...) will remain purely synchronous, would solve this issue/confusion (or whatever it is now). :)

If I am missing something obvious or if we have simply misunderstood each other please explain.

kmruiz commented 5 years ago

The PR https://github.com/vlingo/vlingo-common/pull/11 overlaps and fixes this issue.