Expose internal transitions

[juliusl]

TODO:

• Discuss implementation
• Agree on API's
• Add unit tests

Motivation

In one of my projects I use System.Reactive to drive parts of the state machine. I find InternalTransitions useful for a bunch of discrete steps you'd like fine-grained control on, but that do not represent a super state. I noticed in the current implementation these transitions are not exposed to the OnTransitioned handler, which I find reasonable as they do not represent a transition.

However, it would be useful to expose them in a way so that you could use RX to drive the state machine using internal transitions as well as normal transitions.

Design Choices

• Instead of reusing the current OnTransitioned method, I think it would be appropriate in this case to add a new api, OnTransitionedInternal which is seperate. The reasoning is this sort of change would change the behavior, and break downstream dependencies, which warrants a different API/Event handler all together.

Alternatively, we could add a new constructor parameter, and check a flag to include internal transitions in the original event. However, I feel the current implementation of the constructor is simple and I am hesitant to add more bloat to the constructor. And as for the original intent, RX.Net can easily merge such streams if someone for example needed all events in a single stream.

[HenningNT]

I agree, we should have a callback for the internal transitions.

I am of two minds with regards to how it should be implemented. I am torn between what feels right, and what has the least amount of breakage.

• I feel the right way would be to have one callback for all transitions, regardless of type.
• The way that causes no breakage would be to add a new callback for internal transitions.

Adding a new callback would cause no breakage, but I'm a bit wary. What if someone comes along and asks for Self transition callback, there's already one for internal and one for external. It would make sense to add that as well...
Also, it adds a little bit more code to maintain, and new users might find the two callbacks confusing.

I feel the right way to do it is increase the version number to 6 (then we can also drop netstandard1.x support), change the code so that all 3 transition types uses the one callback.
I think it feels right because there is only one place for transition callbacks (Single Responsibility Principle).
For consumers it would make sense that there is only one callback, I think.

[juliusl]

I agree, we should have a callback for the internal transitions.

I am of two minds with regards to how it should be implemented. I am torn between what feels right, and what has the least amount of breakage.

• I feel the right way would be to have one callback for all transitions, regardless of type.
• The way that causes no breakage would be to add a new callback for internal transitions.

Adding a new callback would cause no breakage, but I'm a bit wary. What if someone comes along and asks for Self transition callback, there's already one for internal and one for external. It would make sense to add that as well...
Also, it adds a little bit more code to maintain, and new users might find the two callbacks confusing.

I feel the right way to do it is increase the version number to 6 (then we can also drop netstandard1.x support), change the code so that all 3 transition types uses the one callback.
I think it feels right because there is only one place for transition callbacks (Single Responsibility Principle).
For consumers it would make sense that there is only one callback, I think.

I agree, if we want to go the single source route a major version bump makes sense because this is behavior changing.

Alternatively, if we wanted to preserve behavior but without the need for a major bump, we could overload OnTransitioned with maybe a filter parameter to preserve the default behavior.

For example something like,

enum OnTransitionFilters
{
Normal = 0,
Reentry = 1,
Internal = 2
}

// Main method:
OnTransitioned(Action<T> callback, OnTransitionFilters filters) 

// Existing Method: 
OnTransitioned(Action<T> callback) => OnTransitioned(Action<T> callback, OnTransitionFilters.Normal)

// Usage for internal/reentrant:
OnTransitioned(() => {}, OnTransitionFilters.Reentry | OnTransitionFilters.Internal)

// Usage for all:
OnTransitioned(() => {}, OnTransitionFilters.Normal | OnTransitionFilters.Internal | OnTransitionFilters.Reentry)

This preserves the single api, while adding some config for the user. Thoughts?

[HenningNT]

I think adding an overload with filtering is good option.
It maintains the present functionality, and adds new functionality.
However, adding filters is probably the solution that adds the most new code to the project. I'm reluctant, since it will have to be maintained...

I'm of two minds, really. But I should probably think of how it will be used, not how much more work we are adding to the project?

[ChaseFlorell]

if this is now a public method, I would be inclined to drop the internal keyword

Suggested change

	public void OnTransitionedInternal(Action<Transition> onInternalTransitionAction)
	public void OnTransitioned(Action<Transition> onTransitionAction)