dotnet / dotnet/docs

CA1507 Suppression guidelines are too limited

Open
#28,793 1 comment 0 reactions 0 assignees View on GitHub
dotnet-fundamentals/svc
Dominant language
No language data
Stars
4.8k
Forks
6.1k
Avg merge
15h 21m
Merged PRs (30d)
370

Description

The documentation states the following for "when to suppress" the rule:
> It's safe to suppress a violation of this rule if you're not concerned about the maintainability of your code.

This is not only extremely shortsighted, but also naive. There are situations where the warning does not make sense and definitely should be suppressed and the result of the suppression is a _gain_ of maintainability, not loss.

I went through one such scenario yesterday: I had a parameterized unit test method that was validating the implementation was throwing the correct exception. Something along these lines (not real code but shows the issue):

```csharp
[InlineData(1)]
[InlineData(2)]
[InlineData(3)]
public void DoingStuffWithValueThrowsArgumentOutOfRangeException(int someValue)
{
// Arrange
var myObject = new MyObject();

// Act
var doingStuffWithValue = () => myObject.DoStuff(someValue);

// Assert
doingStuffWithValue.Should()
.ThrowExactly()
.WithParameterName("someValue");
}
```
Note how the name of the argument on the test method (`someValue`) matches the name of the argument of the method under test. Visual Studio raises CA1507 in this case, asking me to replace the "someValue" constant with `nameof(someValue)`.

That however is a _bad_ refactor: it creates coupling between 2 unrelated, but coincidentally equally named, variables. By following the advice here, I'd be making my test depend on something that it should not: an external argument on the test. If someone refactored my test later and decided to change the test method argument name for any reason (say, readability, or semantics), the assertion would suddenly start to fail.

I think this suppression guideline should be updated to be less naive and more realistic: there are _clearly_ situations where applying this rule's fix is ill-advised and should not be followed, and that does not mean "the developer just didn't care about maintainability" in that case.

If I'm honest, the current wording even sounds ridiculous: "who would ever not care about maintainability in the first place?". It looks unprofessional in a way.

---
#### Document Details

⚠ *Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.*

* ID: 4956d3f2-6966-3a9e-dc29-ba2a61dba3cf
* Version Independent ID: 79e5268e-8cf5-cde5-d8cd-7e5bf469ca02
* Content: [CA1507: Use nameof instead of string (code analysis) - .NET](https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1507)
* Content Source: [docs/fundamentals/code-analysis/quality-rules/ca1507.md](https://github.com/dotnet/docs/blob/main/docs/fundamentals/code-analysis/quality-rules/ca1507.md)
* Product: **dotnet-fundamentals**
* GitHub Login: @gewarren
* Microsoft Alias: **gewarren**

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.