Test and refactor graph + a lot more

[charlierudolph]

• Started out testing and refactoring the graph class
• Implemented TransitionConfigurationSet which is in charge of keeping track of the mapping between start_state, end_state, event, and transition configurations.
  • Allowed for removal of StateMapping
• Had to refactor StateMachine more than anticipated as was getting into infinite call cycles around method_missing.
  • Simplified method_missing and removed __sm from the private methods
• Started using EndState::Matchers in the tests instead of reaching into the implementation
• Removed blocked configuration as events aren't guaranteed to pertain to only a single TransitionConfiguration

Code Climate here. Test Coverage now at 100%

[coveralls]

Coverage increased (+1.42%) when pulling 7f2d8eec21f5c683b6dc2843058c5c130f38340c on charlierudolph:cr-testAndRefactorGraph into 0aa214a979160d8e0a209ffd1d54c802374cad8d on Originate:master.

[alexpeachey]

So a few things.

First, if the graph stuff was a separate PR I would merge that right in.

Second, the changes the StateMachine have some issues. I definitely appreciate the time and effort you've put into this but there is some stickiness.

The reason all those methods were in the format __sm_* was because the StateMachine class is a delegator. As such we need to minimize the footprint of the class so we don't accidentally override methods on the object we are decorating. While unlikely our methods would, there is a much lesser chance if they have a weird and distinctive signature. I would be very surprised to find someone using the gem that had a method in their stateful object called __sm_event but not that surprised to find someone with a method event. Also, somewhat minor, but I'm not a fan of introducing an if/elsif/else structure in method missing when it had a nice structure with guards.

Also, on the removal of the blocked feature. I do like this feature, but I understand that it can't exist as is because there can be multiple transition definitions for a single event and we wouldn't know which one should be used. So I'm ok with removing the feature because it doesn't work as someone might expect it to. However, I'd like to try and rescue it. To rescue it I think we could add another macro method. Maybe something with a signature like handle_blocked :event_name, 'My message.' which then ties the message configuration to the event itself. This can and should be a separate undertaking.

Next Steps

1. Extract the graph bits into a separate PR. Those looked good and were needed to not have that be broken with the latest changes.
2. Extract the removal of the blocked feature into a separate PR. That has surprising functionality as is and shouldn't exist.
3. Leave the _sm_* methods and method_missing as is for now.
4. If you want to tackle the new blocked feature, cool, if not, I'll open an issue around it.

[charlierudolph]

So why does the state machine need to be a delegator? That seems unnecessary to me. The only things that are delegated are state and state=. The persistence concluder uses save but it could be passed the actual object instead of the state machine.

Along the lines of breaking this up.

I can put the blocked stuff into its own PR. Aside from that everything is pretty much dependent. I can rename the private methods back but thats about all. I can't put method_missing back the way it was as I was hitting stack level too deep error.

[alexpeachey]

It needs to be a delegator because that's the pattern we are using. You should be able to wrap your object with a state machine and then use it as you always have and/or use the additional state related methods. It's designed to be a decorator. Much like you can wrap your models with a Draper decorator for presenting in the view. You could even wrap your model with both a Draper decorator and a State Machine. And then further wrap it with some other decorator.

EndState is about composing objects by wrapping them up creating layered onion-objects.

OK, so method missing has to change because a recent change that was made broke it? Everything seems to work on master right now, what is causing the stack level too deep for you? I'm curious what is throwing it. I don't doubt it is there though. So if method_missing needs to be reworked to avoid it, please put the _sm_* methods back and make method_missing with guards like it was previously but avoid the stack level error. I like the fact that we bail to super right away if we are not dealing with a predicate or an event.

[charlierudolph]

Okay. Could a possible future change be moving more logic out of the class so their is no chance the private methods clash? Basically StateMachine just adds the few methods and the modified method_missing and then put the rest of the logic elsewhere. That way can keep the code more readable and have no chance of clashing.

I needed to clean up what method_missing called in order to make sure it didn't call it again. Basically when updating the code that checked if the method was an event it called state which called method_missing again. I kept the same logic in there but made the checks more explicit.

[alexpeachey]

Yes, I actually think if we move forward with resolving the issue I opened up it could reduce our machine down to just having method_missing. Or at least very close to it. Basically if we incapsulate the configuration of the machine in a separate class then it opens up either having that class handle a lot of this or having some additional classes, like perhaps a StateMachineInspector class that takes a configuration and can be asked various things by method missing (i.e. do you have a predicate method called this?) We could also have a class like StateMachineController class that takes a configuration and can be asked to do things like transition.

All of that is just stream of conscious rambling so not necessarily how it would actually be done but yes, we can reduce the footprint of the actual machine for sure. In an idea world it would only have method_missing.

[charlierudolph]

I can't make the PR for the graph + TransitionConfigurationSet, until #26 is merged because the blocked message fails on that branch.