WIP: Adding sendgrid as another publishing handler

[andresfib]

I am working on sending emails and not only tweets from diffengine. To achieve that I have standardized the twitter handler, and created a new handler for sendgrid. For now standardization has made me remove the per feed setup of twitter tokens. If this is required, I'd appreciate for suggestions on how to move that forward.

This passes @nahuelhds tests (after some modifications) but nor the tweets nor the mails have been tested in the wild yet, as I have still to put some more work

[edsu]

This idea is 💯 awesome! When I saw @nahuelhds introduced the idea of a TweetHandler I had the same thought that there could be other types of publication methods. I see that you've flagged this as a draft. I'm curious what @nahuelhds thinks since this extends some of his recent work.

[nahuelhds]

Hi! It's a great idea to have N handlers to choose from.

I've seen the code, I rather don't change the twitter code at all. I've seen that you moved the token from the feed configuration and that breaks the ability to publish in different accounts from the same Twitter app. The way you move the token enforce to use the same TW account to tweet everything and that is exactly the opposite of my use case (and I think is the diffengine case as well as I've based the idea from their config yaml)

In case you want to tweet from the same account I'd create a configuration like this

twitter:
  consumer_key
  consumer_secret
  access_token: 
  access_token_secret: 
feeds:
  - name: 
    url:
    twitter:
      access_token:
      access_token_secret:

And leave the access_token and access_token_secret inside the feeds as well. And if the feed doesn't have an access_token/secret then use the general ones for tweeting.

That way you'll cover both cases: tweet everything from the same account (your case) and tweet one account per feed (my case -and the one that diffengine uses-)

[nahuelhds]

These are some observations for discussing. I do not pretend to have the final words here 😁

About the publisher_handlers approach I rather instance the TweeterHandler and the SendgridHandler and send them as parameters to the methods as the Twitter one does now. Why? Because the injection dependency pattern. It makes you explicit what are you using and makes the tests easy.

Another thing is that I wouldn't use the config.yaml values for instantiating the classes dynamically. It's hard to understand. I prefer doing this

# config.yaml
feeds:
  - name: 
    url:
    twitter:
      access_token:
      access_token_secret:
    sendgrid:
      some_config:
      another_one:

And then in the process_entry method, when a diff is detected, and after or before the twitter.tweet_diff go with a sendgrid.mail_diff and perform a similar validation as it's done in the tweet_diff method where is there is no token present, then do nothing.

This way the code keeps simple and then we can think of an interface or a similar strategy for adding more handlers in the future. I wouldn't overthink it too much right now, if I have to choose.

I'm from the school where I rather write clearer code, even if it's duplicated, than efficient code that doesn't let you understand at first sight what it really does. What is "efficient" if it is not clear when you read it, right?

[andresfib]

Just continuing the discussion, and happy to learn in the process I am fine with no overthinking the implementation, and I will simplify it. Still, I'd say that in my implementation the handlers are still injected to the methods, only in a list.

Regarding the use of the config file to declare the handlers, indeed, it felt "efficient" to declare in advance which handlers are used and not keep trying to mail or tweet when one of the handlers has not been configured at all, as may be the most common case. If only, logging all the errors may get out of hand because of the missing configuration when trying to mail/tweet.

Comments/advice?

[nahuelhds]

Hello again, Andres! If the intention is to avoid the warnings, I'd say to only try tweeting if token is present, that way the process will never reach to the token not found exception (but keep the check/raise code inside the method). A similar path could be used for the mailing.

That way you traverse the list of handlers and only execute the ones you really can and avoid unnecessary executions, exceptions and warning loggings.

I'd divide the PR in two steps:

1. Including the Sendgrid handler
2. Refactor the handlers processing.

That way is clearer the needed actions towards that direction. Doing both things at the same time in the same PR, I think that could make the integration harder,.

What do you think?

[andresfib]

Thanks for the reply and the comment, Nahue. Wouldn't it be nice if the main method is agnostic about the details of each handler?

In any case, you are right with the 2 step process, I'll move towards getting the sendgrid handler working. I'll make the config to be optional, either per feed or global.

We can discuss the handler processing later. Thanks

[nahuelhds]

Wouldn't it be nice if the main method is agnostic about the details of each handler?

Yes. Totally. I want that too. But I prefer to apply the KISS rule: "keep it stupid simple". I think the "simple" definition here is to make SendGrid work in a way that's backward compatible with what it's currently on the master branch.

Then we can go towards the efficiency of having an agnostic handler and work it while you have your mailer working and I have the twitter bots working from the very same version of diffengine 😁