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

CA1303 is more irritating than helpful #2933

Closed
RobinHood70 opened this issue Oct 13, 2019 · 7 comments · Fixed by #3429
Closed

CA1303 is more irritating than helpful #2933

RobinHood70 opened this issue Oct 13, 2019 · 7 comments · Fixed by #3429

Comments

@RobinHood70
Copy link

RobinHood70 commented Oct 13, 2019

Analyzer package

Microsoft.NetCore.Analyzers

Package Version

v2.9.6 (Latest)

Diagnostic ID

CA1303

Repro steps

  1. Create a function with string text as a parameter.
  2. Call the function with a string literal.

Expected behavior

Should trigger only on developer-specified parameter names, or only those tagged with the Localizable attribute. As is, it's more irritating than helpful.

Actual behavior

Triggers every time, whether you want it to or not, and is not configurable in any way without turning off the entire rule.


This was previously mentioned here but the user never followed up on it. Two or three separate rules would be a good start, and being able to configure the parameter name list would be extremely helpful. The System.Exception customization in #2602 doesn't really do anything when you're working with (or designing) libraries that modify language-invariant resources.

The use of the name "text" as a parameter is ubiquitous in libraries that have nothing to do with UI or other language-specific resources. If, say, I were programatically generating code and the program had an AddText(string text) method, I would certainly never want to localize "foreach" to "pourchaque"! Similarly, as a developer, I don't want to be limited to having to design AddText(string txtCuzRoslynAnalyzersSaysNoT_e_x_t) functions, just to be sure I don't use the word "text" in a function where that's the most logical name, and don't hit on any names that might get added to the list later on.

It's a well-intentioned rule, just not a very practical one as it stands now.

@codeaphex
Copy link

Irritating is an understatement.
How can I get rid of this:
image
...without disabling the rule?
I guess localizing isnt common for log messages?

@Evangelink
Copy link
Member

@mavasani Do you have an opinion on this? I tend to agree with the various comment, I think we should report on parameters marked the localizable attribute and also on some well-known invocations. We could think of having a user-option to enable more parameters/invocations.

@mavasani
Copy link
Contributor

@Evangelink Lets start with the following:

  1. Add an editorconfig option, say use_naming_heuristic = true/false, which decides if the following code is executed or not:
    // FxCop compat: If a localizable attribute isn't defined then fall back to name heuristics.
    static bool IsLocalizableByNameHeuristic(ISymbol symbol) =>
    symbol.Name.Equals("message", StringComparison.OrdinalIgnoreCase) ||
    symbol.Name.Equals("text", StringComparison.OrdinalIgnoreCase) ||
    symbol.Name.Equals("caption", StringComparison.OrdinalIgnoreCase);
  2. Let the option be false by default to avoid false positives.
  3. Please file an issue on https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1303?view=vs-2019 to update the documentation to mention this new configuration option starting 3.x release of the package.

@IndigoHealth
Copy link

I'm struggling to understand my options for configuring this warning. Everywhere in my code where I throw an exception with a string literal for the message, I get this warning. Is there a way to configure CA1303 to suppress this construct (code that calls a constructor on a class that inherits from system.Exception)?

@IndigoHealth
Copy link

Also, an editorial cleanup. The documentation language reads:

You can configure whether parameters or properties name containing "Text", "Message",
or "Caption" will trigger this rule.

The syntax should be corrected to read:

You can configure whether parameters or properties whose name contains*...

@JoanComasFdz
Copy link

JoanComasFdz commented May 12, 2020

I have a project in ASP .NET Core 3.1 where i added the nuget package Microsoft.CodeAnalysis.FxCopAnalyzers version 3.0.0 and the use_naming_heuristic and excluded_type_names_with_derived_types don't seem to be working with Microsoft.Extensions.Logging.LogginExtensions or Microsoft.Extensions.Logging.ILogger, so I always get the warnings.

This is my .editorconfig file:

[*.cs]

# CA1303: Do not pass literals as localized parameters
dotnet_code_quality.CA1303.use_naming_heuristic = false
dotnet_code_quality.CA1303.excluded_type_names_with_derived_types = ClassA|LoggerExtensions|ILogger

I have a class ClassA that gets an ILogger<ClassA> injected:

internal class ClassA : IClassA
{
    private readonly ILogger<ClassA> _logger;

    public ClassA(ILogger<ClassA> logger)
    {
        _logger = logger;
    }

    public void Initialize()
    {
        _logger.LogInformation("Initializing...");
    }
}

When calling the logger with a string, I get the CA1303 warning.

I tried with true/false, I tried also adding ILogger<> , ILogger<T> and ILogger<ClassA> but nothing makes the warning go away.

Am I misunderstanding this functionality and configuring wrong or is there possible bug here?

@JoanComasFdz
Copy link

It's a bug already fixed in version 3.3.0-beta1.20261.6.

Source: MVC Core LogCritical etc. messages should not trigger CA1303 #3254

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

Successfully merging a pull request may close this issue.

6 participants