s1kx/unison

Event interfaces

s1kx opened this issue · 1 comments

s1kx commented

As of now, event handling has been improved with the events subpackage, but actual usage is still not pretty.
Event hooks still have to deal with Event interface{} in DiscordEvent and then do type assertions and type casting (in case they are subscribed to multiple events). Needless to say, things like this are not idiomatic:

func onChatlogEvent(ctx *unison.Context, dv *events.DiscordEvent) (bool, error) {
	var m *discordgo.Message

	// Check event type
	switch ev := dv.Event.(type) {
	default:
		return false, nil
	case *discordgo.MessageCreate:
		m = ev.Message
	}

	// ...
}

Instead, I want to propose creating interfaces for groups of related events.

If you look at the event/types.go, you see that there are a lot of events that are related to the same subject (MessageCreate/Update/Delete).
Most of these events in discordgo also only embed the underlying struct (e.g. struct MessageCreate { *Message }). Some discordgo events also don't contain a timestamp, which might be useful in cases where actual event processing is delayed.

To improve the event hook API and making the code more testable, I believe that we should:

  • Rename current types in the events package, so that FooEvent and every other exported type ending with Event will be an interface
  • Create an interface for all discord event classes rather than exporting the current DiscordEvent wrapper type
  • Add timestamps to all events and make all event interfaces export an Action() or Operation() method that reflects the nature of the event

Example code:

type discordEvent struct {
	// current DiscordEvent code here

	// ReceivedAt is the timestamp when the event was received
	ReceivedAt time.Time
}

type DiscordEvent interface {
	// Type returns the type identifier of the event
	Type() EventType
	// Event returns the discordgo event, which has to be typecasted for usage
	Event() interface{}
}

// PersistenceAction is an enum reprenting the default actions covered by most event groups
type PersistenceAction int
const (
	CreateAction PersistenceAction = iota
	UpdateAction
	DeleteAction
)

type MessageEvent interface {
	DiscordEvent

	Action() PersistenceAction

	Message() *discordgo.Message
}

Note:
Type-specific events such as MessageEvent should probably still define their own Action type.
However, this will lead to problems comparing e.g. PersistenceAction values and MessageAction values.
Whether it's worth having an Action() method I am not entirely sure about yet, since the action is already clear from Type(). There might be benefit to having a Action() reside in DiscordEvent and define ALL possible actions in a single enum. Opinions please!

Now the EventHook type can be adjusted to contain callbacks for any class of events, and the Events list will be made obsolete, since EventDispatcher can now simply check if a callback exists:

// EventHook is an application-level type for handling discord requests.
// All callbacks are optional, and whether they are defined or not
// is used to determine whether the EventDispatcher will send events to them.
type EventHook struct {
	// current EventHook fields here

	// OnEvent is called for all events.
	// Handlers must typecast the event type manually, and ensure
	// that it can handle receiving the same event twice if a type-specific
	// callback also exists.
	OnEvent func(ctx *Context, ev events.DiscordEvent) error

	// OnMessageEvent is called for every message-related event.
	OnMessageEvent func(ctx *Context, ev events.MessageEvent) error

	// OnConnectionEvent ...
	// OnUserEvent ...
	// OnChannelEvent ...
	// OnGuildEvent ...
}

This will simplify most use-cases of event hooks, avoiding type assertions and type casting when not explicitely required.

The trade-off here is that EventHooks will be fired on actions they might not want to handle. I think the performance impact would be minimal however and worth the improvements to the packages API for users and testability.

s1kx commented

While on the subject, now might be a good time to consider also moving all event logic into the subpackage. Seems like the hooks and dispatcher logically belong there, building one logical unit, separate from the chatbot core.
The only dependency back to the rest of the project is the Context parameter in the callbacks. We could either change this to ctx interface{}, or use variadic parameters (as in OnEvent func(ev events.DiscordEvent, args ...interface{}) error.