Relax AV1555 so that named parameters are permitted when invoking external libraries

[dennisdoomen]

The following use of named parameters when invoking AddApplicationInsights should be allowed by AV1555

builder.Services.AddLogging(loggingBuilder =>
{
    if (!string.IsNullOrWhiteSpace(connectionString))
    {
        // Enable logging to the Application Insights service.
        loggingBuilder.AddApplicationInsights(
            configureTelemetryConfiguration: config => config.ConnectionString = connectionString,
            configureApplicationInsightsLoggerOptions: options => { }
        );
    }
});

Whether that external library is a package build by the same developer isn't relevant.

See also discussion https://github.com/bkoelman/CSharpGuidelinesAnalyzer/issues/145

[bkoelman]

I think I've seen a matching Resharper/Rider setting for named lambda parameters, would be nice to use matching terminology so it can be automated. I just got back from vacation, will take a look in the next few days.

[dennisdoomen]

I also like this heuristic. https://stackoverflow.com/a/443507/253961

[bkoelman]

The Resharper settings for controlling this are documented here.

For lambda's, here's how to set it up:

Which results in the following addition to the .DotSettings file:

<s:String x:Key="/Default/CodeStyle/CodeFormatting/CSharpCodeStyle/ARGUMENTS_ANONYMOUS_FUNCTION/@EntryValue">Named</s:String>

I've tried this setting out on a codebase, but the results are pretty disappointing because it affects many LINQ methods.

Examples (before):

_parameterReaders.FirstOrDefault(reader => reader.CanRead(parameterName));
propertySelectors.Select(selector => CreatePropertyAssignment(selector, lambdaScope, context));
Values.All(selectors => selectors.IsEmpty);
expression.Elements.OrderBy(element => element.Relationship.PublicName);
AppDomain.CurrentDomain.GetAssemblies().Any(assembly => ...
currentState.FieldsMatched.Sum(field => field.PublicName.Length + 1);

Examples (after):

_parameterReaders.FirstOrDefault(predicate: reader => reader.CanRead(parameterName));
propertySelectors.Select(selector: selector => CreatePropertyAssignment(selector, lambdaScope, context))
Values.All(predicate: selectors => selectors.IsEmpty);
expression.Elements.OrderBy(keySelector: element => element.Relationship.PublicName);
AppDomain.CurrentDomain.GetAssemblies().Any(predicate: assembly => ...
currentState.FieldsMatched.Sum(selector: field => field.PublicName.Length + 1);

In my opinion, turning this setting on isn't worth the extra noise it introduces. This makes me wonder how common it is for a method to take multiple lambda arguments? Aside from AddApplicationInsights (defined here), the only one I can think of is the Enumerable.ToDictionary overload-system-func((-0-1))-system-func((-0-2)))) that takes both a key selector and a value selector.

Assuming this happens only rarely, a Resharper suppression preserves the names while still allowing to auto-format the full solution:

private IDictionary<string, object?> Demo(IDictionary<string, ParameterNode> parametersByName)
{
    // ReSharper disable ArgumentsStyleAnonymousFunction
    return parametersByName.Values.ToDictionary(
        keySelector: parameter => parameter.Name, elementSelector: parameter => parameter.Value);
    // ReSharper restore ArgumentsStyleAnonymousFunction
}

Either way, I think the coding guidelines should dictate a non-ambiguous style instead of relaxing the current one by allowing (but not requiring) named arguments for lambdas. That would result in multiple styles within the codebase (depending on who wrote the code), which is what we're trying to prevent.

A simpler solution is to use a local function to improve readability (two variants, where the function name clarifies the lambda purpose):

private IDictionary<string, object?> Demo(IDictionary<string, ParameterNode> parametersByName)
{
    return parametersByName.Values.ToDictionary(SelectKey, GetElementSelector());

    static string SelectKey(ParameterNode parameter)
    {
        return parameter.Name;
    }

    static Func<ParameterNode, object?> GetElementSelector()
    {
        return parameter => parameter.Value;
    }
}

CSharpGuidelinesAnalyzer could be changed to require named lambda arguments when the called method declares more than one lambda parameter (and forbid using a named lambda argument when there's only one).

I would welcome more use cases and feedback from users before changing this rule, so we can properly define it.

[dennisdoomen]

Thanks for the extensive analysis.

Either way, I think the coding guidelines should dictate a non-ambiguous style instead of relaxing the current one by allowing (but not requiring) named arguments for lambdas. That would result in multiple styles within the codebase (depending on who wrote the code), which is what we're trying to prevent.

I don't agree with that. Although I appreciate a certain amount of consistency, I don't want to be dogmatic about it. Just like the use of var (or not), it is sometimes hard to come up with a hard rule. In the end, it is not about consistency, but about readability. If it helps the readability to use named parameters when invoking lambdas, then by all means, use them.

Suppressing AV1555 for those particular cases is one way of allowing that, but I'm my experience, all those pragmas can make the code quite noisy. In those cases, I tend to disable the rule entirely.

[bkoelman]

Surely the guidelines are not hard rules, if that's what you mean. It's usually fine to deviate when there's good motivation. I just don't get your motivation on why using named parameters is more readable in your opinion. And when you consider it good practice to use named parameters. And how frequently it applies in a codebase. Can you answer these?

Personally, I would write the code as:

builder.Services.AddLogging(loggingBuilder =>
{
    if (!string.IsNullOrWhiteSpace(connectionString))
    {
        loggingBuilder.AddApplicationInsights(
            telemetryConfiguration => telemetryConfiguration.ConnectionString = connectionString,
            applicationInsightsLoggerOptions => { }
        );
    }
});

which matches existing guidelines and is readable enough for me. It also resonates with the general guidance to prefer helpful (as opposed to meaningless) names for identifiers, such as types, members, variables, etc.

[dennisdoomen]

That isn't bad at all. And with that I mean: that's a great alternative that would address my concerns.