Update adapter.go

[patrikduerrenberger]

First of all: I really like your joe-bot and the adapters that come with it.
Thanks for your great work!

However, I ran into a bit of a problem currently with the slack-adapter:
In one of my projects I use your slack adapter inside a custom joe-bot adapter.
This custom joe-bot adapter several custom send methods which used to use the Send()-method of your slack-adapter.

Since I now want to additionally send slack-blocks, when sending messages to slack, I need to be able to hand over a slack blocks message option to the slack API. However this is currently not possible, because the slack API is hidden inside your slack BotAdapter. But with offering a SendWithSlackOpts(...)-method, this could become easily available.

What I used to do:

func (a *slackAdapter) broadcastSend(text, channel string) error {
	return a.eventsAPIServer.Send(text, channel)
}

What I would like to do:

func (a *slackAdapter) privateSend(parsed parsedText, channel string) error {
	blocks, err := a.getSlackBlocks(parsed)
	if err != nil {
		return fmt.Errorf("could not get slack blocks: %w", err)
	}
	opt := slack.MsgOptionBlocks(blocks...)
	return a.eventsAPIServer.SendWithSlackOpts(parsed.text, channel, opt)
}

I wonder whether It would be possible to offer such a SendWithSlackOpts(...)-method?
Or is there anything speaking against it?

[fgrosse]

Hey Patrik,

I think you could be most flexible if your custom adapter maintains its own *slack.Client instance. This pattern is documented at https://joe-bot.net/recipes/slack/

Hope this helps but let me know if you have further questions :)

[patrikduerrenberger]

Hey Friedrich

Thanks for your answer.

You are of course right. I can use that pattern.
However, since I use the EventsAPIServer instead of the "regular" joeslack BotAdapter, this would make things a bit more complicated than in your example code (maybe not a lot, I did not think that one through yet tbh😃).

But my point is: I can understand, if you want to keep your slack adapter minimal and thus don't want to have too many exported BotAdapter methods "polluting" the code, but I cannot really understand it, if it is justified with this argument

Usually the answer is, that we want to keep the generic Adapter interface minimal and thus we cannot add direct support for this feature since it would make it very hard to port it over to other chat adapters (e.g. to IRC).

Offering a SendWithSlackOpts(...) would not result in an extension of the Adapter interface and you would also don't have to port it over to any other adapters. It would just be like a slack BotAdapter exclusive "AddOn" method, that one can use, if it comes in handy or just ignore, if one does not need it. Wouldn't it?

[fgrosse]

So I gave this some more thought and while you are right that adding this function works well without making the Adapter interface bigger, I'm still hesitant. Every extra function in this package is a liability on testing, documenting and general complexity. E.g. right now it is still missing documentation, introduces duplication and lowers the test coverage.

While all of that can be solved, I wonder if you gave it another thought about implementing this on your side by having your own reference to a *slack.Client. This should also work even if you are wrapping the EventsAPIServer type instead of the regular BotAdapter and would offer the most flexibility for you

If you think it doesn't work or would introduce a lot of complexity on your side I would be interested in some more details, maybe a minimal example that showcases your difficulties?

[patrikduerrenberger]

Yes, I can see your point now. You are absolutely right.

I was assuming that writing the test is trivial. Basically it is the same test as for Send(), plus adding some mocked slack.MsgOptions. But I am not sure whether that assumption holds. And regardless of whether it holds, someone still has to do it. Same goes for documentation and solving the code duplication issue. (I would offer to do all of that of course).

But sure, I will give it another thought about implementing this on my side and give you some feedback, should I run into difficulties.

[patrikduerrenberger]

(...) by having your own reference to a *slack.Client. This should also work even if you are wrapping the EventsAPIServer type instead of the regular BotAdapter and would offer the most flexibility for you

Yes, it worked and it did not introduce a lot of complexity on my side. So great!😃

However, I still think it would be useful for the user of the slack-adapter to offer such a SendWithSlackOpts(...).
The user would not need to look under the hood of the slack-adapter too much and could still do a lot of stuff already without having to maintain his own slack client.

So if you ever happen to want to introduce such a method, I would be willing to help with the TODOs still left for that to happen.

Best and thanks for you help.