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

Generics #86

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from
Draft

Generics #86

wants to merge 33 commits into from

Conversation

majst01
Copy link
Contributor

@majst01 majst01 commented Jul 7, 2022

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.

@coveralls
Copy link

coveralls commented Jul 7, 2022

Coverage Status

Coverage decreased (-0.3%) to 93.366% when pulling 2925185 on majst01:generics into 0b86adf on looplab:main.

@maxekman
Copy link
Member

maxekman commented Jul 7, 2022

Really cool! I’ll take some time to read it soon.

@majst01
Copy link
Contributor Author

majst01 commented Jul 9, 2022

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]{
Copy link
Member

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?

Copy link
Contributor Author

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 ?

Copy link
Member

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.

Copy link
Contributor Author

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.

fsm.go Show resolved Hide resolved
@majst01
Copy link
Contributor Author

majst01 commented Jul 11, 2022

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.

@maxekman
Copy link
Member

maxekman commented Jul 12, 2022

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..

https://go.dev/blog/v2-go-modules

@majst01
Copy link
Contributor Author

majst01 commented Jul 12, 2022

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..

https://go.dev/blog/v2-go-modules

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
Up to you

@majst01
Copy link
Contributor Author

majst01 commented Jul 27, 2022

Hi @maxekman

How should we proceed ?

@maxekman
Copy link
Member

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)

@maxekman
Copy link
Member

I published v1.0.0 now, so all is prepared if this should target v2.

https://github.com/looplab/fsm/releases/tag/v1.0.0

@majst01
Copy link
Contributor Author

majst01 commented Dec 23, 2022

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.

@victorkt
Copy link

victorkt commented Feb 5, 2024

Hi, I'd be interested in using this library with generics. Are there any plans to release these changes as v2?

@majst01
Copy link
Contributor Author

majst01 commented Feb 5, 2024

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

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.

4 participants