ConfigurationMethod selector dependent on definition order

[nbaztec]

Issue
As of now ConfigurationMethod selector is vaguely dependent on definition order of the configuration methods. Consider the following configuration class:

MyLoggerConfiguration.cs

static class MyLoggerConfiguration
    {
        public static LoggerConfiguration DemoLoggerConfiguration(
            LoggerSinkConfiguration loggerSinkConfiguration,
            IEnumerable<string> pathFormat)
        {
            return null;
        }

        public static LoggerConfiguration DemoLoggerConfiguration(
            LoggerSinkConfiguration loggerSinkConfiguration,
            string pathFormat)
        {
            return null;
        }

        public static LoggerConfiguration DemoLoggerConfiguration(
            LoggerSinkConfiguration loggerSinkConfiguration,
            bool usesPathFormat)
        {
            return null;
        }
    }

appsettings.json

{
  "Serilog": {
    "WriteTo": [
      {
        "Name": "MyLogger",
        "Args": {
          "pathFormat": "test",        
        }
      }
    ]
  }
}

Current Behavior
Currently the code will try to get one of the 3 DemoLoggerConfiguration methods. As of now it will select the two DemoLoggerConfiguration instances with the matching parameter name pathFormat - however it will use the .First() or the top one and try to invoke it and causing an TypeCastException to IEnumerable<string> from string. The problem being that it ignored the other method which was a valid candidate.

Temporary Workaround
As of now we can reorder the methods so the method with string pathFormat appears before the other method that has it as IEnumerable<string> but it is easy to miss and not a very elegant approach since the order shouldn't matter.

Expected Behavior
The configuration selector should've picked and invoked the 2nd DemoLoggerConfiguration which defined the parameter as a string since it is the most straightforward choice, irrespective of the position where it's defined.

PR
#132

[MV10]

While you raise a valid point, I'm not sure PR #132 focus on string types is necessarily the right decision. The package already has code which can attempt type conversion, probably selection should be based upon matching names and the first argument type which can perform a successful conversion from the provided value, which would be quite a bit more complicated.

This line attempts to convert the config value to the parameter type:
https://github.com/serilog/serilog-settings-configuration/blob/dev/src/Serilog.Settings.Configuration/Settings/Configuration/ConfigurationReader.cs#L331

And this is the conversion function used for string config values (the other being objects, which are generally config subsections):
https://github.com/serilog/serilog-settings-configuration/blob/dev/src/Serilog.Settings.Configuration/Settings/Configuration/StringArgumentValue.cs#L34

[nbaztec]

I'd considered that approach as well but after thinking about it I reckoned that if a user has a method that accepts string overloads they would have parsing logic in that method either way.

As such the reliance on string comes into affect only when there's a string override - which is the norm for JSON configurations.

Thoughts?

[MV10]

Yes but LoggerConfiguration extensions are designed with configuration-through-code in mind. These various config packages are sort of add-ons. But still, if a matching string argument exists, I can agree that ought to be selected first. I'm just trying to decide whether this type of fix needs to go beyond that to more robustly match with other types.

I may be overthinking it, though. I have not had much success imagining other scalar overload scenarios that seem realistic.

Where did you encounter this problem? Was it essentially what you represented in the example?

[nbaztec]

Yes indeed, that was my thought process as well. It was encountered while I was patching a bug in the Udp sink package and the maintainer expressed that it was an easy to miss error which they frequently encountered themselves.

Basically, in true configuration-by-code manner, the first and original method accepted IPAddress and a secondary overload, to read JSON configuration and accept hostnames + IPs, would accept string. The string value would then be parsed to an IPAddress within the function body or remain a hostname. However due to the ordering, the value "localhost" was always being tried to be cast as an instance of IPAddress.

While one can argue that they could've used separate names for IP address and hostname parameters, that however would've been a workaround if the key is simply stated as "remoteHost".

[MV10]

I suppose the general argument is that if a matching string argument exists, that's going to be the best choice given that all config values are string values first and foremost.

There is a conflict with #128 but I'll merge your PR once that is resolved.