fsm icon indicating copy to clipboard operation
fsm copied to clipboard

Allow transitions in callbacks

Open annismckenzie opened this issue 3 years ago • 5 comments

This allows state transitions to happen in enter state and after event callbacks by adding the possibility of "starting" a state machine and have it execute multiple state transitions in succession, given that no errors occur.

The equivalent code without this change:

var errTransition error

for errTransition == nil {
	transitions := request.FSM.AvailableTransitions()
	if len(transitions) == 0 {
		break
	}
	if len(transitions) > 1 {
		errTransition = errors.New("only 1 transition should be available")
	}
	errTransition = request.FSM.Event(transitions[0])
}

if errTransition != nil {
	fmt.Println(errTransition)
}

Arguably, that’s bad because of several reasons:

  1. The state machine is used like a puppet.
  2. The state transitions that make up the "happy path" are encoded outside the state machine.
  3. The code really isn’t good.
  4. There’s no way to intervene or make different decisions on which state to transition to next (reinforces bullet point 2).
  5. There’s no way to add proper error handling.

It is possible to fix a certain number of those problems but not all of them, especially 2 and 4 but also 1.

The added test is green and uses both an enter state and an after event callback. No other test case was touched in any way (besides enhancing the context one that was added in the previous commit).

The relevant test is here: https://github.com/alfaview/fsm/blob/94fb353ef273319b14928aa3a04ab4e01a58d6da/fsm_test.go#L735-L774

This fixes #36. It also fixes #38. And it fixes #43. Finally, it fixes #61.

annismckenzie avatar Jul 29 '22 14:07 annismckenzie

Coverage Status

Coverage increased (+0.1%) to 93.735% when pulling 99d5c76dec95fc2d6401d46ee8b2005d97be295e on alfaview:allow-transitions-in-callbacks into 54bbb61e7a6ce42ea71b1c21777fad84da99069d on looplab:main.

coveralls avatar Jul 29 '22 14:07 coveralls

I'll still want to put the two tests into examples. I'll finish this up hopefully by next week. But I wanted to give @maxekman something to look at.

annismckenzie avatar Jul 29 '22 14:07 annismckenzie

Thanks for the PR! Looks good on a quick review. Ping me when you feel done.

maxekman avatar Jul 29 '22 19:07 maxekman

Hey guys :) Any progress on this one?

lupuszr avatar Aug 23 '22 06:08 lupuszr

Hey hey, @annismckenzie any updates on this?

rasha08 avatar Sep 20 '22 10:09 rasha08

Hey hey, @annismckenzie any updates on this?

Sorry about the long wait. I'm scheduling some time during work hours to finish this.

annismckenzie avatar Sep 26 '22 10:09 annismckenzie

Thanks for the PR! Looks good on a quick review. Ping me when you feel done.

@maxekman I pulled out the two tests into their individual examples. I feel like this is done and it would be nice if you took a look. I'm really sorry about the long wait.

annismckenzie avatar Sep 27 '22 15:09 annismckenzie

Nice work! Are you happy with merging this as it is?

Yup, pretty happy with it. Go ahead. 🎉

annismckenzie avatar Oct 02 '22 10:10 annismckenzie

This solves a problem that I encountered (along with the existing chain of issues) as a new user of the library. Thanks @annismckenzie for the initial work on the fork as well!

GuessWhoSamFoo avatar Oct 03 '22 06:10 GuessWhoSamFoo

Thanks a lot @annismckenzie! Great work on this!

maxekman avatar Oct 06 '22 21:10 maxekman

This solves a problem that I encountered (along with the existing chain of issues) as a new user of the library. Thanks @annismckenzie for the initial work on the fork as well!

Awww, somehow this got lost in my inbox – thanks! 🙃

annismckenzie avatar Oct 31 '23 20:10 annismckenzie