Consider returning all values from actions for a triggered transition

[iamgodot]

Consider returning all values from actions for a triggered transition

[fgmacedo]

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.

[oniboni]

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 😺

[iamgodot]

@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.

[iamgodot]

@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.

[iamgodot]

Thanks for the thorough explanation! @fgmacedo

[fgmacedo]

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

[iamgodot]

@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.

[fgmacedo]

Do you agree with this approach?

If an event is triggered externally, the state machine library will return any non-null callback values. If there is only one non-null value in the list, it will be returned directly without being wrapped in a list.

[oniboni]

Do you agree with this approach?

If an event is triggered externally, the state machine library will return any non-null callback values. If there is only one non-null value in the list, it will be returned directly without being wrapped in a list.

I've added this behavior to my PR.

Sadly, it does not comply with the expected behavior in the test here. I guess this is only there, to ensure backwards compatibility. Therefore I didn't touch it 😨

[iamgodot]

@fgmacedo Actually I'm thinking about something more dedicated. For now it's complicated to figure out the mapping relationship between an action and its returned value. Thus a list of key-value tuples(or maybe an ordered dict) would be more friendly, when it comes to using or debugging by the result.

Regarding the performance, I suppose a lazy-evaluated iterable such as a generator could offer more, particularly if the result gets ignored in most of the cases.

[fgmacedo]

@iamgodot thanks for this different perspective.

Can you elaborate more on a concrete example where this map can be useful?

Regarding using a lazy evaluate structure, I think that It will not help here, since the SM should run all callbacks any way, and for the SM the results does not matter, but the results will already be collected in a structure that is already evaluated.

[iamgodot]

@fgmacedo Sorry for the last suggestion, I was looking at this code result_list = [r for r in result_list if r not in EMPTY_RETURN_VALUES] in the pull request and started thinking away... It may help a bit but pretty pointless.

As for the use of a mapping structure, I'd say with multiple items in the list, it's much easier to figure out where a certain value comes from, rather than checking back all the action callbacks, which most likely happens upon debugging I think.

A simple example of this structure would be [('before_transition', 1), ('on_transition', 2), ...].

[iamgodot]

@fgmacedo On second thought, things would get a bit more complex with linked event triggering described in here, in this case mapping key may need to include more info e.g. an event reference.

[fgmacedo]

@iamgodot An option that could be added to the work @oniboni is doing on his PR #380 (on another PR problably), is to include a flag on state machine instance that instead of returning the result directly, we return the EventData.

Considering that EventData.return is a dict-like object, we could write something like this:

        event_data.return["before"] = transition.before.call(
            *event_data.args, **event_data.extended_kwargs
        )

        if source is not None and not transition.internal:
            event_data.return["exit"] = source.exit.call(
                *event_data.args, **event_data.extended_kwargs
            )

        event_data.return["on"] = transition.on.call(
            *event_data.args, **event_data.extended_kwargs
        )
        
        ...

And then, the returned value calculated by this flag:

        if event_data.machine.return_event_data:
            return event_data
        return event_data.return.build_cleaned_return()

Where:

class ReturnValues(dict):
    def build_cleaned_return(self):
        result_list = [r for r in self.values() if r not in EMPTY_RETURN_VALUES]
        if len(result_list) == 0:
            return None
        elif len(result_list) == 1:
            return result_list[0]

        return result_list

Note: Each Callbacks.call() returns a list, so the typing is something like this

ReturnValues: Dict[str, List[Any]]

What do you think?

[fgmacedo]

@iamgodot What if we return a namedtuple? So we keep the "list-like" behaviour with optional name access.

[iamgodot]

@iamgodot What if we return a namedtuple? So we keep the "list-like" behaviour with optional name access.

I'm not sure how a namedtuple would contain arbitrary number of result values. Or you mean namedtuple for every ResultValue in the list?

[iamgodot]

@fgmacedo Sorry for delaying on this discussion, I hope you're doing well. Here's my proposal:

1. For detailed return value, we build a dict in which the callback name maps the corresponding value list, e.g. {"before_foo": [...], "on_foo": [...]}. And generic callbacks don't have to show up if not defined.
2. About the flag to enable this enhanced return value, I figure it could be more flexible to add it on the send method, also they're more attached in the nature. What do you think?

I'm ready to work on the code changes if this makes sense to you. Please let me know your thoughts on this, thank you :-)