Closed dwmunster closed 5 years ago
I was thinking about this for a bit and while my initial reaction was that this is a really good idea I'm still a bit hesitant and first want to share my thoughts on it.
Generally you are right that many chat adapters offer reactions one way or another. However how that really looks like may still differ between them. Some might accept simple smileys (e.g. :)
), some emoji references such as :fire:
or :smile
, some might require you to pass reaction names without enclosing :
(like Slack) and for some the same reaction might have different names (e.g. +1
vs thumbsup
).
So with that in mind I think, adding reactions on the Adapter interface level will either introduce a lot of complexity or alternatively the caller will have to know what kind of adapter they are dealing with (e.g. Slack vs Telegram).
Instead I want to encourage users to take a different route to access custom adapter features to get all functionality that their specific adapter provides without having to unify it all under a single large interface. In my own bots so far I am using the following approach successfully:
package main
import (
"context"
"github.com/go-joe/joe"
"github.com/nlopes/slack"
)
// Bot is my own custom bot. It may use all features of the slack API in its commands.
type Bot struct {
*joe.Bot
Slack *slack.Client // might be nil if we are running without slack
}
func New(ctx context.Context, conf Config) (*Bot, error) {
b := &Bot{
// I allow my bots to choose which adapters to run with (e.g. debug
// mode only with a CLI adapter and production with the actual slack
// adapter).
Bot: joe.New("joe", conf.Modules()...),
}
if conf.SlackToken != "" {
// If slack integration was enabled we create a slack client instance
// and store the reference in a field of the custom Bot type. Note that
// for the used slack client this does *not* mean we have more than
// one persistent connection to the API but it might depend the API
// you are using. In this case its absolutely fine.
b.Slack = slack.New(conf.SlackToken)
}
// Another approach could be to make a type assertion on b.Bot.Adapter.
// This might be useful if you really only want a single client to the API
// the adapter is talking to. However it requires that the used adapter
// exposes the adapter somehow (e.g. as field or via a function). I would
// usually opt for simply creating a new instance to decouple yourself as
// much as possible from the used adapter details (e.g. who handles closing
// of the adapter).
//
// if adapter, ok := b.Bot.Adapter.(joeslack.BotAdapter); ok {
// b.Slack = adapter.API
// }
b.Respond("do stuff", b.DoStuffCommand)
return b, nil
}
func (b *Bot) DoStuffCommand(msg joe.Message) error {
if b.Slack == nil {
// In case this command does not even make sense without your custom
// functionality you may want to return early in the command.
return msg.RespondE("Cannot do stuff because Slack integration is not enabled")
}
// Access to specific functionality is accessible via an extra field on the Bot
err := b.Slack.AddReaction("fire", slack.NewRefToMessage("ChannelID", "123"))
if err != nil {
return err
}
// You can still use all regular adapter features easily
msg.Respond("OK")
return nil
}
I suppose such patterns should be better documented though. Before I invest any time in that though I want to hear your thoughts. How well does this answer your specific use case you currently have in mind?
I appreciate writing up some of your thoughts and an example. While this doesn't mollify all of your concerns, I believe some of the specific issues raised may have resolutions.
some emoji references such as
:fire:
or:smile
, some might require you to pass reaction names without enclosing:
(like Slack) and for some the same reaction might have different names (e.g.+1
vsthumbsup
).
Adapters are precisely the ones who would know whether a smile
reaction is spelled smile
, :smile
, :smile:
, etc. At the (bot) user level, that could be resolved by a method taking just the emoji name and letting the adapter decide how that translates to the particular service. As for the different names, I believe that most services probably support a significant shared base of emojis. For example, they may not all alias +1
to thumbsup
, but they all probably have thumbsup
.
Instead I want to encourage users to take a different route to access custom adapter features to get all functionality that their specific adapter provides without having to unify it all under a single large interface. ... I suppose such patterns should be better documented though. Before I invest any time in that though I want to hear your thoughts. How well does this answer your specific use case you currently have in mind?
Between those options, I think that a way to expose the adapters would be the way to handle this. Dealing with the raw service API at the bot level is just about the opposite of what I (personally) would like.
While adding reactions to messages would be nice, one of my main goals was to start thinking about how reactions are handled in the bot. There are some really neat things that reactions enable. As an example: You could build a bot that is triggered with a number of options for where to go for lunch, people can vote on those options with reactions, then when it's time to leave, the bot can DM respondents to remind them that it's time to go. This idea extends something like lunchbox on slack.
Right now building my own go-hubot, thinking about dropping it in favor of go-joe. We would like to take advantage of such function as reaction, but it is indeed not obvious thing to deal with.
Let me break the problem a bit.
Ok, we have abstraction around the services, so we have one interface to handle any message using any service. Considering reactions, we have 3 possible cases:
Reaction formats can be as many as chat services we support. It is not possible to unify interface with providing all capabilities for all the services: some smileys won't be supported in A, some in B, but sometimes we cannot even make a reaction, since service does not support it.
So, in general, there are two approaches to such problem in CS:
We can leave react to be implemented by either handlers developers or some-kind-of-plugin developers. But when community is not as big as Linux or JS, such segregation will leave us with many unsupported or inconsistent, incompatible packages. It is already the case for JS community, since it became TOO segregated, with packages like "isbool" or "isnull".
Opinionatedness is what drives many good solutions. Even when community is not happy about it.
In general, we always should have such approach available. And it is available now, as shown by @fgrosse . But I would prettify it a bit, like so(abstract):
switch adapter := b.Adapter.(type) {
case Slack:
Must(adapter.AddReaction("fire", slack.NewRefToMessage("ChannelID", "123")))
case IRC:
Must(adapter.SendMessage(msg.Author + " yeah!"))
}
In general(hehe), it is a good idea. Which has one idea in mind: simplification of the higher-end codebase by abstracting away platform-specific implementations.
So, the question we discuss here, should sound like that:
Is reaction API so important, that we need to add another abstraction layer and move responsibility from handlers developers to adapter developers?
Keep in mind: every method interface provides is abstraction by itself, so adding one more abstraction won't be that much. Also, it is much easier to support adapters rather to deal with some unpopular-handler maintainer.
I think the more and easier concepts will be provided for the handlers developers, faster these concepts will be adopted, standardised. The only thing that matter is cost of implementation in adapters. And I think React is too simple to question it.
Back to the cases:
Then, we will have abstraction. Like so: msg.React(emoji.ThumbsUp)
. So yeah, as part of the abstraction, we will need emoji library(I built one, which regenerates from the https://emojipedia.org, which is not a bit deal, will make it public soon).
Then, we each adapted will need to implement it one way or another, depending on the capabilities. emoji.ThumbsUp
has shortcuts property, are like :+1:
, so some services can send it as is, some can strip a colon, covering cases 2, 3, and any possible formats. If format does not support certain emojis, it should provide fallback mechanism, providing emoji replacement.
If service does not support that, reaction could be treated as reply message. Covering first case.
Beside the add
action, most services have remove
, list
, get
methods. Which cannot be provided with every adapter. Still, could be adapted as no-op. But it is a question for another time.
In opinion, React could be implemented and it makes sense. The only question is resources. If we can afford to implement it and its fallback cases in all adapters, it worth a time.
Thanks @iorlas for taking the time to summarize the problem and give some valuable input.
Is reaction API so important, that we need to add another abstraction layer and move responsibility from handlers developers to adapter developers?
That is exactly the question: does this feature (a general react API) add enough value to justify the complexity it introduces into the code base. Also how will we deal with similar requests to add other adapter features that might be specific to subset of adapters (e.g. #24). I do not want to eventually end up in a situation where we have a gigantic Adapter interface that tries to provide all kind of features individual adpaters might have. The worst case scenario would be a very broad Adapter interface that is hard to implement and reason about while still only giving users only a limited subset of the wanted functionality because it is an abstraction).
However having said all of that, I think you made some good arguments and currently I am leaning towards giving it a try to come up with a good design :)
So yeah, as part of the abstraction, we will need emoji library(I built one, which regenerates from the https://emojipedia.org, which is not a bit deal, will make it public soon).
Lets be careful with adding dependencies by default. With Joe I tried to have a design where users can opt-in to dependencies by explicitly adding them as modules. This way you only get what you actually want but if you do not need a feature you do not need to vendor it. I like the idea in general though (emoji constants and generating them from a somewhat canonical source). Maybe this could be some kind of Module which people could import only if they actually need it?
In opinion, React could be implemented and it makes sense. The only question is resources. If we can afford to implement it and its fallback cases in all adapters, it worth a time.
So in summary from my side I agree it would be a nice feature and implementing it as a first class citizen in Joe will expose this feature in a consistent way to handler developers. I think next I will implement a POC to understand all implications and to find a design that limits the complexity while still being easy to use.
If somebody else also wants to give it a try feel free to hack something together that we can look at some compiling code and reason about. However if you want to do that please keep in mind that I think at this point we are still playing with the idea and it might not get merged at all. I simply do not want to move too hasty with this and I hope the issue is not extremely pressing on your side
So I played with this a little :).
At first I tried the opt-in approach of implementing this via another module. I wanted to see if I could make it work in an elegant way that would users allow to only import this feature (and potentially any complexities) if they need it. I then realized that while this would be possible, pretty much everybody would import it when they use an adapter that supports it. For instance if we want adapters to fire an event in case a user added a reaction (my own use case) and we want to use constants for that then the adapter would have to import them always to compile. Maybe I could have realized this earlier but it means that very few people would actually opt-out and then its really not worth the complication of a module.
The other approach is to simply add it to go-joe/joe (initially proposed in this issue). I implemented that quickly without worrying too much about the adapter a that point and came up with an optional interface and pretty much this implementation in the Message
:
func (msg *Message) React(reaction reactions.Reaction) (ok bool, err error) {
adapter, ok := msg.adapter.(ReactionAwareAdapter)
if !ok {
return false, nil
}
return true, adapter.React(reaction, *msg)
}
Returning the bool gives control to the caller about how they want to handle the situation at the cost of making the call a bit more complex. Right now I am a bit undecided if that idea is good or bad (will most people ignore the bool anyway?).
reactions.Reaction
here would be an import from github.com/go-joe/joe/reactions
where we could generate code to have a mostly complete list of reactions as was suggested earlier as well. The adapters that want to implement this interface could either directly pass the constant string values to their API or alias some of those in case their API names things a bit differently. I hope its not that much of a deal as I initially thought.
I pushed both variants under the following two branches:
Let me know your thoughts about this and thanks for being patient with this feature and allowing a bit of discussion around it :pray: If we agree I think I can provide the complete implementation with slack as first adapter supporting it relatively quickly.
Hey!
A bit overwhelmed on my lap. Reading your messages here, have some thoughts, but need some time to give suggestions. Here just few throws in for ya:
I mean, if one needs simple, thin slack/telegram bot implementation, it would be better to implement it w/o any middleman. So, if I wanna use such things as hubot or gojoe, thinness won't be my concern, but functionality, ease of integration, ability to go wild and make some awesome things just by using a method or two.. it is what valuable for me.
So, maybe it is not a best idea to make it "thin". What, maybe, is needed, is to find a balance between chaos and "thinness".
adapter, ok := msg.adapter.(ReactionAwareAdapter)
Looks solid! That is the most go-ish way to do so. The key downside would be a need to make N x-ability interfaces: for each ability, there should be an interface. Also, each time we want to use such action, we will be forced to add a check.
Direct usage would be "sexier" I guess. Which mean ease of use, ease of discover such new actions/features, easier codebase for the end user.
func DoWork(msg joe.Message) error {
must(msg.React(":+1:"))
must(time.Sleep(1 * time.Second))
must(msg.Respond("Job is done"))
must(msg.React(":love:"))
}
vs
func DoWork(msg joe.Message) error {
if adapter, ok := msg.adapter.(ReactionAwareAdapter); ok {
must(adapter.React(":+1:"))
}
must(time.Sleep(1 * time.Second))
must(msg.Respond("Job is done"))
if adapter, ok := msg.adapter.(ReactionAwareAdapter); ok {
must(adapter.React(":love:"))
}
}
What I'm thinking right now, why some should use GoLang instead of Python or NodeJS? For me, key points are:
Ease of use - Any stack developer can utilise it, any team member can go wild and implement any new feature or update any functionality: add cats pics when something is deployed, add :+1: icon when deployment is in progress.
Code quality - With typescript we have a hell of templates, which fears some developers. With Python we can types already, but it is optional and quirky. GoLang is a really good balance here.
Type assertion syntax is what makes it really hard to understand, to discover new features. So my first suggestion would be to return NotSupported error instead. But I'm still thinking...
What are your thoughts?
Hey @iorlas thanks for your feedback again. I think you are right that simplicity is very important.
Based on this I chose to fully implement the simpler approach and also remove the boolean return value I was mentioning earlier (i.e. to check if the adapter actually supports this feature).
I think with #30 the interface is pretty much as it was requested. Please feel free to give it a critical review if you have time.
I like the implemented approach quite a bit. I'll probably start building the rocket.chat adapter soon and that should give me some additional thoughts.
Okay, I will wait for your then to see if you have any thoughts or concerns after you completed your implementation :+1:
The feature is available via Joe v0.9.0 :tada: :tada: :tada:
Summary
Many chat services (e.g. Slack, Rocket.Chat, Mattermost, etc.) offer the ability to "react" to messages with emoji. It would be nice to be able to make use of that capability.
Basic example
Motivation
Using reactions is an easy way to acknowledge that a command has been received without cluttering up the chat with intermediate results. Simple commands could even skip output entirely and just use reactions.
Potential Issues
Not all Adapters will be able to support reactions (e.g. CLI Adapter). A decision would be necessary for how to handle reactions in this case. My thought would be to fail silently (optionally with a logger warning) and to document that reactions are optional for adapters. An alternative would be to error if not available and to offer a capability to test if reactions are available
ReactionsEnabled
or perhapsmsg.ReactIfEnabled
.Edit 1
Perhaps a pragmatic way to handle the absence of reactions would be a
ReactOrRespond
function that would add a reaction if able, respond if not.