-
Notifications
You must be signed in to change notification settings - Fork 312
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
Generics #86
base: main
Are you sure you want to change the base?
Generics #86
Conversation
Really cool! I’ll take some time to read it soon. |
OK, now callbacks are transformed to generics as well, no more string concat and matching. This is all obviously breaking for existing users. |
{Event: Open, Src: []MyState{IsClosed}, Dst: IsOpen}, | ||
{Event: Close, Src: []MyState{IsOpen}, Dst: IsClosed}, | ||
}, | ||
fsm.Callbacks[MyEvent, MyState]{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can more types be inferred here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven’t used generics in Go so much, it just felt there was some repetition in specifying the types. Thought maybe more types could be inferred, but that was more of an open question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, type inference is the most complicated part in the generics of the go compiler, i tried making this a bit lighter by have a fluent interface for the FSM creation, something like:
fsm := fsm.New(initialState).WithTransitions(transitions).WithCallbacks(callbacks)
Methods usually make inference easier for the compiler, but it didnt work out.
As this PR is obviously much to big to review, i would have no problem maintaining a fork of it in our org (https://github.com/metal-stack) because we will most probably use the generic version of it in our main api. But if you are willing to take it, this will of course be much appreciated and i will try hard to make this possible. So if you have time to comment, do so, i will help pushing it. |
Great work so far! I’m thinking we should maybe release it as a v2, WDYT? In that case I think it needs to go into a v2 folder.. |
Yes sure, i can modify my branch to act as v2, but i am not so much a fan of a /v2 subdirectory, what about a branch ? Main points to the generic version and v1 branch is the "old" one. A sample can be seen here: https://github.com/urfave/cli |
Hi @maxekman How should we proceed ? |
I think I prefer a branch too, but haven’t read too much about it. Let’s start with that. As long as the package name contains v2 it should be fine. See here for update on releasing a v1: #82 (comment) |
I published v1.0.0 now, so all is prepared if this should target v2. |
Hi @maxekman This is great news, i am short in free time ATM, polishing it and make it ready for v2 will take a bit effort though. Will come back once time allows. |
Hi, I'd be interested in using this library with generics. Are there any plans to release these changes as v2? |
This was the plan, but with the very last changes to master right before, i got so many conflicts that i was not able to fix them in a couple of hours. If you want to try, would be great |
Hi Max,
This is just a POC to see what it might look like, not so convinced if its worth the effort.
Callback handling is hard to transform because it depends on string prefixes of events or states. Callback handling is now generic as well.