-
Notifications
You must be signed in to change notification settings - Fork 806
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
proposal: update handler function signatures to use an interface for the session #662
Comments
This is somewhat related to #564. |
Could we not stay with a pointer but use an interface instead of the raw structure ? |
@Hinara I'm not sure I completely understand, are you asking about changing the Assuming I am understanding that correctly, you pretty much never want something to be a pointer to an interface unless you plan on swapping out the thing that's boxed inside of that interface. We don't plan on doing that inside of
To summarize, we should change the type from |
hmm ok I didn't remembered the way golang work with intefaces ^^' |
Please be aware, this is a breaking API change. This change adds a new `discordgo` type, `Sessioner`, which is an interface that describes the full method set for the `*Session` type. In addition to the new type, this change updates the package to no longer send a concrete `*Session` value in to handlers it invokes as the first argument but a `Sessioner` instead. This is to make testing of handlers much easier, as mock implementations of the Sessioner interface can be passed right in. This also renames the internal state field from `State` to `state`, and adds a new method to `*Session`, `State()`, to expose that internal state to consumers. A test was added to ensure this method works as expected. The example projects were updated where necessary to work with the new API Fixes bwmarrin#662. Signed-off-by: Tim Heckman <[email protected]>
Please be aware, this is a breaking API change. This change adds a new `discordgo` type, `Sessioner`, which is an interface that describes the full method set for the `*Session` type. In addition to the new type, this change updates the package to no longer send a concrete `*Session` value in to handlers it invokes as the first argument but a `Sessioner` instead. This is to make testing of handlers much easier, as mock implementations of the Sessioner interface can be passed right in. This also renames the internal state field from `State` to `state`, and adds a new method to `*Session`, `State()`, to expose that internal state to consumers. A test was added to ensure this method works as expected. The example projects were updated where necessary to work with the new API Fixes bwmarrin#662. Signed-off-by: Tim Heckman <[email protected]>
Please be aware, this is a breaking API change. This change adds a new `discordgo` type, `Sessioner`, which is an interface that describes the full method set for the `*Session` type. In addition to the new type, this change updates the package to no longer send a concrete `*Session` value in to handlers it invokes as the first argument but a `Sessioner` instead. This is to make testing of handlers much easier, as mock implementations of the Sessioner interface can be passed right in. This also renames the internal state field from `State` to `state`, and adds a new method to `*Session`, `State()`, to expose that internal state to consumers. A test was added to ensure this method works as expected. The example projects were updated where necessary to work with the new API Fixes bwmarrin#662. Signed-off-by: Tim Heckman <[email protected]>
I've raised a PR against the develop branch that should accomplish this. |
Please be aware, this is a breaking API change. This change adds a new `discordgo` type, `Sessioner`, which is an interface that describes the full method set for the `*Session` type. In addition to the new type, this change updates the package to no longer send a concrete `*Session` value in to handlers it invokes as the first argument but a `Sessioner` instead. This is to make testing of handlers much easier, as mock implementations of the Sessioner interface can be passed right in. This also renames the internal state field from `State` to `state`, and adds a new method to `*Session`, `State()`, to expose that internal state to consumers. A test was added to ensure this method works as expected. The example projects were updated where necessary to work with the new API Fixes bwmarrin#662. Signed-off-by: Tim Heckman <[email protected]>
I've started to use Discord Go to write a bot, and have found it somewhat difficult to properly test without writing my own shimmed handlers.
This challenge exists because the first argument to handlers is a struct type and not an interface, so a mock implementation can't be provided to assert proper behaviors. The standard library has solved for this exact problem as the first argument to a handler (
http.ResponseWriter
) is an interface, so that alternative implementations can be provided for testing.I'd like to propose a breaking change to the library to update the handler signature to go from
discordgo.Session
being a struct to it becoming an interface. So from:to
The benefit of making this change is that Discord Go handler functions / methods could be tested directly, without needing to write shims for each handler you implement. This would make it much easier to test, and the ease of which could be shown-off in the example projects. It also may not be a major breaking change for some users, if they only use the methods on the type, which would be a good benefit.
One downside of this change is that the size of the
*discordgo.Session
method set is large today. Even using composition, adiscordgo.Session
interface would end up becoming quite large. We would also need to add more methods, getters and setters, to tweak the internal settings.Overall, I think this would be a net-win for the project and would like to encourage its inclusion in the 0.20.0 development cycle.
The text was updated successfully, but these errors were encountered: