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

Refactor open/close logic. Support OnConnect and OnDisconnect. Closes #5. #93

Merged
merged 13 commits into from
Jan 21, 2016
Merged

Conversation

iopred
Copy link
Collaborator

@iopred iopred commented Jan 21, 2016

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:

a := discordgo.New()
a.OnConnect = func(s *discordgo.Session){
// Not called.
}

Instead, there is now an explicit (and simple) call to start listening, the above example is now:

a := discordgo.New()
a.OnConnect = func(s *discordgo.Session){
// Called.
}
a.OpenAndListen()

}
go s.heartbeat(st.HeartbeatInterval)
Copy link
Collaborator Author

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

@iopred
Copy link
Collaborator Author

iopred commented Jan 21, 2016

Please pull and give it a run. I've been running Septapus on this branch all night and things look really great.

@bwmarrin
Copy link
Owner

Reading though this now. I think this touches on some things I'd been thinking about changing, maybe now is a good time.

    • Why would anybody ever call Open without wanting to call Listen/Heartbeat? We could probably just combine those like I decided to do on the Voice side.
    • No idea how to do this, but I really liked how simple New() was. Not sure if there's a magic trick we can use here to keep it as simple as it was before.

@bwmarrin
Copy link
Owner

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.

@iopred
Copy link
Collaborator Author

iopred commented Jan 21, 2016

Cool, I agree, I didn't want to go too wild at first :D

@iopred
Copy link
Collaborator Author

iopred commented Jan 21, 2016

Ok, so here are my thoughts:
New() shouldn't need to listen. It's perfectly reasonable to want to use the rest api without using wsapi. I could see people wanting just to send a message in a cron job or something.

Open/Listen, I like the separation because Open() is blocking, but Listen isn't.
I want to consume the errors from Open, but I can't if it also does listening, as you can't get errors from goroutines.

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.

@bwmarrin
Copy link
Owner

Stop making sense, you.

@bwmarrin
Copy link
Owner

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.

@iopred
Copy link
Collaborator Author

iopred commented Jan 21, 2016

Actually I have been convinced about adding listen to the end of Open, making changes now.

bwmarrin added a commit that referenced this pull request Jan 21, 2016
BREAKING - Refactor open/close logic. Support OnConnect and OnDisconnect. Closes #5.
@bwmarrin bwmarrin merged commit 94b087c into bwmarrin:develop Jan 21, 2016
@bwmarrin bwmarrin added the feature Major feature implementation label Jan 27, 2016
@bwmarrin bwmarrin added this to the v0.10.0 milestone Jan 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Major feature implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants