Add support for IFormatProvider used to convert string to other types

[0xced]

Also introduce a new ConfigurationReaderOptions class to avoid ReadFrom.Configuration() methods exponential growth when adding new options.

All older Configuration() methods go through the newly introduced Configuration(LoggerSettingsConfiguration, IConfiguration, ConfigurationReaderOptions) method that takes an ConfigurationReaderOptions instance.

Older methods explicitly set the FormatProvider option to null in order to preserve backward compatibility.

By using the new Configuration() method, users opt into the new default of having the invariant culture as the format provider.

Note: the = null default value in the Configuration() method taking a DependencyContext has been removed in order to make sure the CS0121 compilation does not occur:

[CS0121] The call is ambiguous between the following methods or properties: 'ConfigurationLoggerConfigurationExtensions.Configuration(LoggerSettingsConfiguration, IConfiguration, DependencyContext)' and 'ConfigurationLoggerConfigurationExtensions.Configuration(LoggerSettingsConfiguration, IConfiguration, ConfigurationReaderOptions)'

Fixes #325

[sungam3r]

Are they mutually exclusive?

[0xced]

Yes. They influence how the assemblies that contains sinks and other types are discovered. At least one must be specified. There are three matching ConfigurationContext constructors and then the configuration reader is created depending on how the context was created:

var configurationReader = context switch
{
    { ConfigurationAssemblySource: {} } => GetConfigurationReader(configuration, context, context.ConfigurationAssemblySource.Value),
    { Assemblies: {} } => GetConfigurationReader(configuration, context, context.Assemblies),
    _ => GetConfigurationReader(configuration, context ?? new ConfigurationContext(), context?.DependencyContext),
};

[nblumhardt]

Looks great; had some thoughts on naming and defaults

[nblumhardt]

new ConfigurationReaderOptions()? The "context" suffix gets a bit overloaded, especially when several of the members are just regular old options.

[0xced]

I must admit that I anticipated on my next pull request which will address #206. I plan to add a new property to get access to the log level switches on that new object:

public IReadOnlyDictionary<string, LoggingLevelSwitch> LogLevelSwitches { get; }

I already have a LogLevelSwitches branch on top of this FormatProvider branch if you want to have a look.

At that point, that ConfigurationReaderOptions would become «bidirectional», i.e. properties to tweak the reader behavior + property to access things (log level switches for now) created after the configuration is read. So naming it context might be more appropriate I think. Maybe ConfigurationReaderContext would be a better choice?

[nblumhardt]

Thanks for the reply! I think even in the "bidirectional" case, it would be more common to see callbacks on the options type, e.g.:

new ConfigurationReaderOptions
{
    OnAddedLevelSwitch = levelSwitch => _mySwitches.Add(levelSwitch)
}

This would save us from needing to worry about the lifetime and thread-safety of other things attached to the options type, or having to explain that the type is bidirectional (and flag which members are "in" vs "out").

(Having named a few things *Context over the years, I've just about always regretted it one way or the other :-) ... for example, I'd love to go back and rename LogContext to the much more descriptive LoggingScope, or ForContext() to WithTag() or just about any other name ... context is used in so many ways that it's hard to guess its precise intention when you come across one.)

What do you think?

[0xced]

Very good point! I went with ConfigurationReaderOptions as you suggested and force-pushed fbc5e72 on top of the dev branch. I'll adapt my LogLevelSwitches branch with your OnAddedLevelSwitch callback proposal and I'll open a new pull request to address #206 once this one is merged.

I also adapted the README to mention since version 4.0.0 instead of since version 3.5.0 as I initially did before the rebase.

Side note: how would you name the HttpContext class? 😂

[nblumhardt]

A lot of people will still want to write ReadFrom.Configuration(config) and skip the redundant extra argument. A breaking change might be fair, here, given that the source for most settings coming from configuration will be JSON, where anything other than the invariant culture is probably just harboring bugs.

What do you think?

[0xced]

Actually, the newly introduced method already supports calling ReadFrom.Configuration(config) without an extra argument since the context parameter defaults to null:

public static LoggerConfiguration Configuration(
    this LoggerSettingsConfiguration settingConfiguration,
    IConfiguration configuration,
    ConfigurationContext context = null)

By the way, I had to remove the default = null in the overload taking a DependencyContext. That's what I tried to explain in my original message but was maybe not very clear.

public static LoggerConfiguration Configuration(
    this LoggerSettingsConfiguration settingConfiguration,
    IConfiguration configuration,
    DependencyContext dependencyContext)

So people just using ReadFrom.Configuration(config) will opt in the new behaviour (invariant culture) by updating to Serilog.Settings.Configuration 3.5.0 and recompiling their app. The only downside is that they will probably not even realise they opt in the new behaviour.

And people using ReadFrom.Configuration(config, … extra argument …) will get an obsolete warning and will be forced to have a look at the new overload and think about how they want to create the new ConfigurationContext object.

[nblumhardt]

Ah, thanks - I missed that. I'm trying to get a few minutes spare again to catch up with this, I'll try to loop back later today :-)

[nblumhardt]

Awesome PR 👍