Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow transitions in callbacks #88

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

annismckenzie
Copy link
Contributor

@annismckenzie annismckenzie commented Jul 29, 2022

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.

This adds 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:
```go
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).
This adds a context and cancelation facility to the type `AsyncError`.
Async state transitions can now be canceled by calling `CancelTransition`
on the AsyncError returned by `fsm.Event`. The context on that error can
also be handed off as described in looplab#77 (comment).
@coveralls
Copy link

coveralls commented Jul 29, 2022

Coverage Status

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

@annismckenzie
Copy link
Contributor Author

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.

@maxekman
Copy link
Member

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

@lupuszr
Copy link

lupuszr commented Aug 23, 2022

Hey guys :) Any progress on this one?

@rasha08
Copy link

rasha08 commented Sep 20, 2022

Hey hey, @annismckenzie any updates on this?

@annismckenzie
Copy link
Contributor Author

Hey hey, @annismckenzie any updates on this?

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

@annismckenzie
Copy link
Contributor Author

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.

fsm_test.go Show resolved Hide resolved
Copy link
Member

@maxekman maxekman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@annismckenzie
Copy link
Contributor Author

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

Yup, pretty happy with it. Go ahead. 🎉

@GuessWhoSamFoo
Copy link

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!

@maxekman maxekman merged commit 3637340 into looplab:main Oct 6, 2022
@maxekman
Copy link
Member

maxekman commented Oct 6, 2022

Thanks a lot @annismckenzie! Great work on this!

@annismckenzie
Copy link
Contributor Author

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! 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants