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

Typed handler with eventcallback #192

Conversation

thestonehead
Copy link

You last review was fair, but let me propose this approach.
If this is also not to your satisfaction, I'll stop.

This way, the support for .netstandard2.0 is maintained, however there's the price of handling different targets.

Dejan Frankovic added 2 commits February 23, 2024 08:06
Added support for TypedEventHandler to use EventCallback
@soxtoby
Copy link
Owner

soxtoby commented Mar 3, 2024

Can't say I'm a fan of this approach either, unfortunately. IEventHandler<T> having two methods for handling the same event seems like a confusing API to me - it's not clear from the interface which method will be called, or how the Handle method should be implemented if the HandleWithContext method is also implemented.

The current way to implement an event handler that requires the EventCallback is:

class MessageEventHandler : IEventHandler {
    public Task Handle(EventCallback eventCallback) {
        if (eventCallback.Event is MessageEvent message) {
            // Handle message
        }
    }
}

and the "improved" version would be:

class MessageEventHandler : IEventHandler<MessageEvent> {
    public Task Handle(MessageEvent slackEvent) => Task.CompletedTask;

    public Task HandleWithContext(MessageEvent slackEvent, EventCallback callback) {
        // Handle message
    }
}

As much as I agree it would be nice to have access to the EventCallback in typed handlers (all else being equal), I don't think the benefit here outweighs the cost in complexity, sorry.

@soxtoby soxtoby closed this Mar 3, 2024
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.

None yet

2 participants