python-statemachine: Consider returning all values from actions for a triggered transition

Hi @iamgodot , you’re right.

This happened because, at the beginning in the primitive versions of the library, there was only one callback for a transition. The showcase was that you could model a state machine much more likely running transitions acting as standard/normal method calls.

Imagine this:

class OrderControl(StateMachine):
    waiting_for_payment = State(initial=True)
    processing = State()
    shipping = State()
    completed = State(final=True)

    add_to_order = waiting_for_payment.to(waiting_for_payment)
    receive_payment = waiting_for_payment.to(processing ) | waiting_for_payment.to(waiting_for_payment)
    process_order = processing.to(shipping)
    ship_order = shipping.to(completed)

    def __init__(self):
        self.order_total = 0
        super().__init__()

    def on_add_to_order(self, amount: float):
        self.order_total += amount
        return self.order_total

This is more exotic but also supported:

class OrderControl(StateMachine):
    waiting_for_payment = State(initial=True)
    processing = State()
    shipping = State()
    completed = State(final=True)

    receive_payment = waiting_for_payment.to(processing ) | waiting_for_payment.to(waiting_for_payment)
    process_order = processing.to(shipping)
    ship_order = shipping.to(completed)

    def __init__(self):
        self.order_total = 0
        super().__init__()

    @waiting_for_payment.to(waiting_for_payment)   # declares the transition while also adding a callback
    def add_to_order(self, amount: float):
        self.order_total += amount
        return self.order_total

For both cases, you can just call the Event like a method, as you’re using normal Python calls:

sm = OrderControl()
assert sm.add_to_order(3) == 3
assert sm.add_to_order(5) == 8
assert sm.order_total == 8

The complexity comes when supporting multiple callbacks for the same event, so my evolutionary approach was: When adding more than one callback option if the user of the library implements more than one, It will start receiving a list instead of the plain return.

This worked well because simple use cases are still able to use a simple API, and was also backward compatible.

I think that many use cases more event-oriented don’t even care about the result, but we need to keep a sane default behavior for those who care.

@fgmacedo How about we keep the result as a list, while each item in it represents a ReturnValue object which attaches the value & name of the action & event data(or something similar). Also we provide a optional flag for this dedicated version of result just like you wrote before.

I’ve actually done this in my fork, but didn’t consider creating a PR yet.

Will do, when I’ve merged the (great!) improvements from 2.0.0 😺

@fgmacedo Sorry for the delay, I needed to take another look at the code.

1. Not trying to nitpick here, but return EventData seems confusing, as a user I’d wonder why gives me such an object. Plus it contains a bit too much data.
2. Maybe adding a flag would make things too complicated? I’m not sure, but I understand it’s about backward compatibility here.
3. In the nested event scenario, I think my concern still stands, particularly for Non-RTC model. The result would be a nested list with the existing logic, which is very natural and understandable. But to improve it, we need identifiers for at least events & callbacks. For example: [('event1.before_transition', 1), ('event1.on_transition', 2), ('event2.on_transition', 3), ...]. And it gets more complex when it comes to something like t1 = a.to(b, after="t1") | b.to(c)(this is from your test code).

I hope I’m making sense here and we are on the same page before talking about next steps.

@oniboni you can change the test, with this new rational, we’ll need a major version bump IMO.

Thanks for the thorough explanation! @fgmacedo

@fgmacedo I just found that when there’s only one value returned, it will be taken out instead of being a list, which seems a bit strange.