Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dennisdoomen opened this issue May 3, 2024 · 6 comments
Labels

Comments

@dennisdoomen
Copy link
Owner

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 bkoelman/CSharpGuidelinesAnalyzer#145

@bkoelman
Copy link
Contributor

bkoelman commented May 3, 2024

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
Copy link
Owner Author

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

@bkoelman
Copy link
Contributor

bkoelman commented May 5, 2024

The Resharper settings for controlling this are documented here.

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

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 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
Copy link
Owner Author

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
Copy link
Contributor

bkoelman commented May 9, 2024

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
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants