-
Notifications
You must be signed in to change notification settings - Fork 822
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
Refactor open/close logic. Support OnConnect and OnDisconnect. Closes #5. #93
Conversation
} | ||
go s.heartbeat(st.HeartbeatInterval) |
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 can't see any reason to keep Heartbeat as a public method, we removed the need to call it manually previously, and it is cleaned up with the Listen() loop on Close().
Please pull and give it a run. I've been running Septapus on this branch all night and things look really great. |
Reading though this now. I think this touches on some things I'd been thinking about changing, maybe now is a good time.
|
https://github.com/bwmarrin/discordgo/pull/93/files#diff-9c7b1115ae6d16ebcb782b325ee2b8b3R567 sendHeartbead should probably use an anonymous struct instead of a map to keep it consistent with the rest of the library. |
Cool, I agree, I didn't want to go too wild at first :D |
Ok, so here are my thoughts: Open/Listen, I like the separation because Open() is blocking, but Listen isn't. I could see the last line in Open being go Listen(), but it seems more obvious to a developer imo. Finally I updated the anonymous struct. |
Stop making sense, you. |
I do actually sort of like the idea of having go Listen as the last section of Open() we could even later add a little code so Open() can check via a chan/bool that the Listen didn't hit some error immediately. I guess the fine line of exposing enough to make it a useful library but keeping it as simple as possible so it's easy for people to "just use". If everyone is just going to end up calling OpenAndListen() then might as well just be Open() unless there's some valid cases where someone would need to call Open and Listen separately. |
Actually I have been convinced about adding listen to the end of Open, making changes now. |
BREAKING - Refactor open/close logic. Support OnConnect and OnDisconnect. Closes #5.
This has a backwards incompatible change to New.
The reason is that with an OnConnect event, there is no time to hook it up, eg:
Instead, there is now an explicit (and simple) call to start listening, the above example is now: