Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

API is potentially redundant #55

Open
tilgovi opened this Issue Mar 5, 2014 · 16 comments

Comments

Projects
None yet
3 participants

tilgovi commented Mar 5, 2014

It seems to me that repoze.sendmail has an SMTP mailer and a sendmail mailer. Both implement IMailer. Why does pyramid_mailer create both and expose itself as a wrapper around both with separate API functions?

Is there a common deployment scenario where one application would want to use both methods to send mail?

It seems to me that this just creates incompatibilities where some module authors will get the mailer and call send() while others will call send_sendmail().

Would it not be better to choose, by configuration, which mailer is used, so that module authors can just call send() and the deployment can choose which is used?

Owner

tseaver commented Dec 10, 2014

This package offers a tighter integration into the Pyramid ecosphere than repoze.sendmail. I can't imagine there is any need for a user who installs this package to think about repoze.sendmail at all, or try to use it directly.

@tseaver tseaver closed this Dec 10, 2014

Owner

mmerickel commented Dec 10, 2014

FWIW I tend to agree that send, send_to_queue, send_sendmail, send_immediately and send_immediately_sendmail, and are not ideal APIs and rather this should be simply hidden behind a send that is defined at configuration-time. At least I find it difficult to see why that's an issue, especially if it accepted a **kw to override the default send.

Owner

mmerickel commented Dec 10, 2014

@tseaver I'm not sure your comment is relevant.. the OP isn't bringing up repoze.sendmail but rather the silly apis on pyramid_mailer.Mailer.

@mmerickel mmerickel reopened this Dec 10, 2014

tilgovi commented Dec 10, 2014

@tseaver added me to the team for this repo a while back. I had been wanting to facilitate a release to get the debug mailer changes out. I'll take a look at this issue if I can, soon, but wanted to get feedback.

How would we feel about simply send, send_immediately and send_to_queue and letting sendmail use be pure configuration?

Owner

mmerickel commented Dec 10, 2014

This sounds acceptable to me but even then send and send_immediately feel like all that are necessary. No?

Owner

mmerickel commented Dec 10, 2014

I guess I can see send_to_queue if send is configured to send directly upon commit of the transaction which it is. I should stop this stream of consciousness.

tilgovi commented Dec 10, 2014

send_to_queue works differently. A separate process is expected to flush that queue.

Owner

mmerickel commented Dec 10, 2014

Sure, but isn't it still transactional? Is it really a good idea to leave it up to the caller whether the email gets send in the background or foreground? Feels like a config-time option.

tilgovi commented Dec 10, 2014

I guess that depends on the intended use.

I could image wanting to process the queue mail differently. It may be bulk mail that needs to go through some tracking service that handles unsubscribe or something. It may need a different SMTP server, to ensure transactional mail doesn't end up undeliverable because your bulk mailings are getting marked as spam. On the other hand, the non-queued sends would be for transactional mail in response to direct user action.

On the other hand, if that's not the intended usage, then I agree it should be a config-time option.

Owner

mmerickel commented Dec 10, 2014

Well there's no way atm to send_immediately_to_queue either.

tilgovi commented Dec 10, 2014

I'm not familiar enough with the repoze mail. Is it the case that both delivery mechanisms delay until transaction commit?

Owner

mmerickel commented Dec 10, 2014

Yeah the DirectMailDelivery (send and send_sendmail) and QueuedMailDelivery (send_to_queue) and are transactional whereas the other mechanisms (immediate-mode) are not.

tilgovi commented Dec 10, 2014

Let's be careful with the language here, too. "Transactional" has a meaning in mailing that is opposite "bulk", in addition to the meaning of "takes effect iff the transaction commits" here. I don't think we've confused this in the thread here yet, but just pointing that out for sanity.

But, let's talk about transactions as in the transaction module. We could have send_to_queue_immediately by using repoze.sendmail.maildir directly.

Right now we seem to have two dimensions (and you rightly point out we are missing a quadrant):

Transaction vs Non-transaction
Now(-ish) vs Queued

We don't have non-transactional, queued.
We do have sendmail/smtp as a third dimension, but I think we all agreed that's unnecessary.
But the other two dimensions might both be good.

At the least, the transaction support seems great, especially because we inherit it with little effort from the repoze package.

And queue vs non-queue feels important because the mail world has this notion of transational vs bulk mail and I can see using it for that difference. But maybe that's the wrong use case and I'm wrong and this should be pure configuration.

So, I think we can check the box for "remove the send_sendmail and send_immediately_sendmail" APIs. No consensus yet on anything else.

Does that sound right?

Owner

mmerickel commented Dec 13, 2016

I'm going to try to resurrect this. Probably via cutting the current pyramid_mailer as 1.0 and then discussing a 2.0 API that standardizes on .send() with config-defined defaults.

IMO pyramid_mailer is currently a poor abstraction because people sending email still need to think about which mailer they want - how many times should this be different in different parts of your app? I'm currently thinking about the following.

class Mailer:
    def send(self, message, **kw):
        """
        Send a message.

        By default, all options are taken from the default mailer (usually
        automatically constructed from the application settings).

        :param immediate: If ``True`` the message will be sent immediately
        on its own transaction.

        :param transaction_manager: Override the default
        ``transaction_manager`` bound to the mailer. This option is mutually
        exclusive with the ``immediate`` option.

        :param queue: If ``True`` the mail will be sent to the local queue
        defined by the ``queue_path``. This will error if no ``queue_path``
        has been defined.

        :param queue_path: Override the default ``queue_path`` setting.

        :param sendmail: If ``True`` the message will be sent using the
        ``sendmail_app`` and ``sendmail_template`` settings.

        :param sendmail_app: The sendmail executable. This value is
        interpolated into the ``sendmail_template`` argument list.

        :param sendmail_template: The command-line argument list invoked.
        If you just need to change the executable then set ``sendmail_app``
        instead.

        """

This would be accompanied by the mail.default_delivery setting which would be direct by default:

mail.default_delivery = direct | queue | sendmail

In general it's possible to make everything immediate if necessary because we can wrap it in a new local transaction manager and call commit - so any delivery mechanism that is transactional can be made immediate.

Does anyone have anything to say about this idea?

tilgovi commented Dec 29, 2016

👍

tilgovi commented Dec 29, 2016

This doesn't restrict any flexibility but it means most people, most of the time, can just call send(message), which is what I think is most valuable. People are used to thinking of kwargs as secondary / if necessary / additional options, but having to decide which of five API calls to make to send a message is what I really disliked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment