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

Add ability to document FSM visually. #9

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Bluebugs
Copy link

I found useful to visualize the FSM as a graph inside github documentation using mermaid. I tried to do that without breaking adding any cost when not generating documentation, but I may have missed something. This doesn't completely cover the full extent of what the FSM can do as I didn't really know yet how to display things, might require further experiment.

@Bluebugs
Copy link
Author

You can see the graph directly on my fork as I incorporated one in the README.md.

@jfbus
Copy link
Contributor

jfbus commented Oct 25, 2022

Thanks for the PR. Let me think a little bit about this... Not a big fan of the breaking change, but I agree that generating graphs would be useful.

@Bluebugs
Copy link
Author

Yeah, I tried to make it so that old code would just work without change (there is no change to tests or example and they all still pass). The main issue I see is the State that is being passed in the callback is the numerical fsm.State one not the extended state, so when using extended state you would have to do some type conversion, but that should prevent existing code from breaking. Anyway I may have missed stuff, that's we do review and discuss things :-)

@andydotxyz
Copy link

I was looking into whether this could be done more simply and I am not sure - there is a lot going on. I did wonder though if the problem of graphing the FSM could be broken into 3 sub-problems:

  1. A "lookup table" solution - something that would avoid all the code generation but would be a little manual.
  2. An automated version of the above which might bring in gostringer again, or reflection
  3. Expanding to support multiple different FSM in a single file.

On the surface it seems like it might be possible to satisfy 1) with something as simple as FSM.Document(stateNames map[State]string, eventNames [Event]string) string - taking this route might mean that (in the short term at least) 3 is a non-issue.

I don't know if this helps or not, but it came to mind as I tried to learn what all the new types were required for.

@jfbus
Copy link
Contributor

jfbus commented Oct 28, 2022

I'm not a big fan of the ExtendedXXX types. I rather like @andydotxyz 's ideas.

  1. Add a lookup table
  2. Add a Document method - alternatives : Document(state_or_event any, doc string) (e.g. Document(StateFoo, "foo")) or with variadic parameters Document(state_or_event_or_doc ...any)(e.g Document(StateFoo, "foo", StateBar, "bar"))
  3. If nothing is found in the lookup table, use reflection to fetch the name

Another idea would be to change Event/State types to any, and let the user define his own type. If the type implements Stringer, use it for documentation, otherwise use reflection to get the name.

@Bluebugs
Copy link
Author

I think reflection only work if we have a structure which won't work with enum. I may be wrong with that, but I couldn't get that to work previously.

I am not to sure what the difference between any and an interface in this case would imply. Would you mind elaborating on the difference between the two and why you would prefer any? It feels to me like we loose all type check in that case.

Also I think a debug and review tool has to be reliable. Relying on manual work for those to break that assumption and a manual lookup table seems like it can only be manual.

@Bluebugs
Copy link
Author

@andydotxyz
Copy link

Another random thought, if lookup is not found and if reflection does not work there is still "Event(4)" which could easily be printed which is not a wholly useless output for those not wanted to code the names in?

@Bluebugs
Copy link
Author

That's the current fallback when using fsm.Event or fsm.State for backwards compatibility. It is much less useful in practice as it is hard to match those value any meaning.

@Bluebugs
Copy link
Author

Another attempt I have tried is to use generics for the State and Event type. So instead of an interface used for the API parameters it become an interface used for constraint. Something like:

type NamedState interface {
   ~int
   fmt.Stringer
}

But I couldn't make it work as the option pattern doesn't know the fsm it is running on, which means it can not infer the type of Event or State properly when they are not part of the parameters of the function. For example fsm.On(e Event) is unable to infer the State type for the fsm and require to do something like that fsm.OnMyLocalStateType for Go to be able to pick things which I think clutter the code massively and obviously break the API.

@jfbus
Copy link
Contributor

jfbus commented Nov 28, 2022

I have been thinking about this, and I would like to try using parser/ast. This would allow generating doc without touching the fsm package.

@Bluebugs
Copy link
Author

This is an interesting idea. Feel free to @ me if you want some review/eye.

@jfbus
Copy link
Contributor

jfbus commented Feb 7, 2023

@Bluebugs sorry for the delay. I played with the Go parser, and it looks we can generate a nice flowchart from the ast. I did a quick and dirty cmd to test it. See PR #10

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

Successfully merging this pull request may close these issues.

3 participants