search

serilog / serilog-sinks-email

A Serilog sink that writes events to SMTP email
Apache License 2.0
73 stars 68 forks source link

Extract e-mail transport from sink #55

Closed mgrosperrin closed 3 years ago

mgrosperrin commented 6 years ago

I have introduced two abstractions:

I modified the EmailConnectionInfo to add a protected internal virtual CreateEmailTransport method to create the IEmailTransport the sink should use. This will allow unit testing the sink without the transport, and also people to use another transport if they want (by subclassing the EmailConnectionInfo).

There is now only one sink and two implementations of the transport.

Regarding PRs #51 and #54, I think it will be simpler to merge this one first and update the others PRs .

fixes #27

PS: I also fix a typo when splitting the To addresses...

mgrosperrin commented 6 years ago

Hi all,

can anyone have a look at this PR? Thanks

adamchester commented 6 years ago

Sorry @mgrosperrin, we’ll try to get back to this soon.

mgrosperrin commented 6 years ago

No problem @adamchester, it was just to not forget it 😄

mgrosperrin commented 5 years ago

Hi @adamchester, I don't know if you had time to read my answer to your comments. Do you prefer I made the IEmailTransport and Email internal, or is it fine to let them public?

nblumhardt commented 3 years ago

Thanks for this PR @mgrosperrin, and sorry about the long, long delay 😊 ... unfortunately things have moved on a bit in the intervening years.

To answer the last question above - Serilog has a very strong leaning towards minimal public surface area (at its scale of use, all public APIs have a maintenance cost that can/does overwhelm our small community team), so we tend to make everything private/internal unless there are critical/compelling reasons to expose a public API.

This PR was mentioned over in #97 as a useful way to get test coverage up in this repository. Are you interested in continuing with it, or would you mind if someone else picks it up now?

mgrosperrin commented 3 years ago

I close this PR because due to some cleanup, I delete the previous repo for this PR.

The new one is here: #98