snoyberg / mime-mail

Compose MIME email messages.
41 stars 43 forks source link

Generalise the API slightly #65

Closed adinapoli closed 4 years ago

adinapoli commented 4 years ago

This small PR generalises a bit the API by introducing a new function called sendMailSESWithResponse which is really what sendMailSES used to be, but it exposes a onResponseStatus callback that can be used to customise the return type, to account for situations where users might want more control over what to do on the Status returned by the HTTP Response, and return something more meaningful than ().

sendMailSES is now re-implemented under the hood in terms of this new function, thus this is not a breaking change as the type signature for sendMailSES stays the same.

@kindaro What do you think?

adinapoli commented 4 years ago

Hey Ignat,

Thanks for getting back to me so quickly. Yes, all good; different projects have different conventions when it comes to adding more (possibly unrelated) stuff to a PR, so I went for the "beefy" approach by default, as the rest is harmless, but I understand where you're coming from :)

I will polish and rebase the PR tomorrow so that it delivers only the API change, and will fire other PRs with the stylistic changes as I see fit.

Thank you!

On Wed, 2 Sep 2020, 18:49 Ignat Insarov, notifications@github.com wrote:

@kindaro requested changes on this pull request.

There are several independent changes here:

  • White space fix — should go in its own commit or pull request.
  • Cabal support — should go in its own pull request.
  • Editor support (tags) — should go in their own pull request, and maybe Emacs tags can be added too.
  • The feature itself — this looks good.

This is of course all minutiae, but still I would appreciate if you can address it. We must make every effort to keep the history clear and complete.

In .gitignore https://github.com/snoyberg/mime-mail/pull/65#discussion_r482189253:

@@ -1,3 +1,5 @@

*.swp

dist

.stack-work/

+tags

+dist-newstyle

This should go to a pull request of its own, together with cabal.project.

In .gitignore https://github.com/snoyberg/mime-mail/pull/65#discussion_r482190345:

@@ -1,3 +1,5 @@

*.swp

dist

.stack-work/

+tags

This line is best omitted from this pull request. Rather, we can craft a new pull request with all sorts of editor specific stuff, such as emacs and vim tags.

In cabal.project https://github.com/snoyberg/mime-mail/pull/65#discussion_r482192759:

+packages: ./mime-mail

  • ./mime-mail-ses

This should go to a pull request of its own, dedicated to adding support for building with Cabal.

In mime-mail-ses/Network/Mail/Mime/SES.hs https://github.com/snoyberg/mime-mail/pull/65#discussion_r482198394:

-sendMailSES :: MonadIO m => Manager -> SES

+

+sendMailSES :: MonadIO m

  • => Manager

  • -> SES

I understand your desire to fix this inconsistent white space, however, being independent of all other changes, this one should go in its own «white space» commit.

In mime-mail-ses/Network/Mail/Mime/SES.hs https://github.com/snoyberg/mime-mail/pull/65#discussion_r482215133:

         -> L.ByteString -- ^ Raw message data. You must ensure that
                         -- the message format complies with

                         -- Internet email standards regarding

                         -- email header fields, MIME types, and

                         -- MIME encoding.

         -> m ()

-sendMailSES manager ses msg = liftIO $ do

+sendMailSES manager ses msg =

  • sendMailSESWithResponse manager ses msg checkForError

+-- | @since 0.4.3

+-- Generalised version of 'sendMailSES' which allows customising the final return type.

+sendMailSESWithResponse :: MonadIO m => Manager

This here is asking for a line break before => for consistency with the other white space change above.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/snoyberg/mime-mail/pull/65#pullrequestreview-480976785, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADL5M375STTMDH3UDRFBTTSDZZSVANCNFSM4QS75IVQ .

adinapoli commented 4 years ago

@kindaro Ok, I think we should be in business now, let me know if this is more palatable now!

kindaro commented 4 years ago

@adinapoli  I released to Hackage, hope everything matches your expectations. Let me know if anything is out of order.

adinapoli commented 4 years ago

Marvellous, thank you very much @kindaro !

I will try this out next week and report back, but from a quick Hackage inspection I think everything is fine 😉