Update dependencies and add overloads to allow custom SMTP Server Ports #48
Conversation
|
Thanks for the PR, Doug 👍 The backwards-compatible overload is sound, but over time if we use this approach we end up having to create a lot of potentially overlapping method signatures to cope with new options (i.e. each overload needs a new overload for each new feature). There's an alternative documented in this comment that should work here - would it be possible to switch to that approach to add an Thanks again! |
|
I'll definitely take a look. |
|
All right... that took a lot more effort than I anticipated. I followed the example suggested and I think I've got something that works that will also allow for the future. But the more that I worked with this, the more that I think it needs a significant rewrite and version bump. |
| @@ -176,6 +166,7 @@ public static class LoggerConfigurationEmailExtensions | |||
| /// <param name="mailSubject">The subject, can be a plain string or a template such as {Timestamp} [{Level}] occurred.</param> | |||
| /// <returns>Logger configuration, allowing configuration to continue.</returns> | |||
| /// <exception cref="ArgumentNullException">A required parameter is null.</exception> | |||
| [Obsolete("New code should not be compiled against this obsolete overload"), EditorBrowsable(EditorBrowsableState.Never)] | |||
| public static LoggerConfiguration Email( | |||
There was a problem hiding this comment.
Should this one be obsolete? I think the port is present on EmailConnectionInfo, so no change should be needed here AFAICT.
There was a problem hiding this comment.
You can't configure EmailConnectionInfo from appsettings.json with Serilog.Settings.Configuration. EmailConfigurationInfo completely defeats the point of having default values for the parameters.
| @@ -138,28 +133,23 @@ public static class LoggerConfigurationEmailExtensions | |||
| string fromEmail, | |||
There was a problem hiding this comment.
Is this overload missing an [Obsolete]?
| @@ -214,25 +204,115 @@ public static class LoggerConfigurationEmailExtensions | |||
| /// <param name="mailSubject">The subject, can be a plain string or a template such as {Timestamp} [{Level}] occurred.</param> | |||
| /// <returns>Logger configuration, allowing configuration to continue.</returns> | |||
| /// <exception cref="ArgumentNullException">A required parameter is null.</exception> | |||
| [Obsolete("New code should not be compiled against this obsolete overload"), EditorBrowsable(EditorBrowsableState.Never)] | |||
| public static LoggerConfiguration Email( | |||
There was a problem hiding this comment.
As per above, this one seems like it should be fine, via EmailConnectionInfo.
| string mailSubject = EmailConnectionInfo.DefaultSubject, | ||
| string outputTemplate = DefaultOutputTemplate, | ||
| string mailServer = null, | ||
| int smtpPort = EmailConnectionInfo.DefaultPort, |
There was a problem hiding this comment.
The new parameter needs to be last, so that existing calls with unnamed parameters continue to compile.
| /// <param name="subjectFormatter">ITextFormatter implementation to write log entry to email subject.</param> | ||
| /// <returns>Logger configuration, allowing configuration to continue.</returns> | ||
| /// <exception cref="ArgumentNullException">A required parameter is null.</exception> | ||
| private static LoggerConfiguration BuildSink( |
There was a problem hiding this comment.
Nit: the codebase avoids explicit private/internal modifiers.
|
The more I work with this sink the more frustrated I become. This issue is a complete showstopper for me as fixing risks breaking existing usages. The suggestion here of splitting the sink has a lot of merit and I think is the correct way to go. I'm going to close this PR... |
|
Yep, know the feeling 👍 Logging directly to SMTP email is often a pretty awkward experience - email has too many points of failure of its own to be a good channel except in the most dire circumstances. If you have access to a monitoring tool that can do email alerting with some kind of aggregation of events, plus rate limiting, that's the way to go. |
See #33 for more.
Did not change any existing API, just added overloads to preserve backwards compatibility.