Rule summary

What is the problem

Two or more attributes appeared within the same set of square brackets.

For test projects, its nice to have the two attribute next to each other- the two attributes "belong" to each other.

[Theory, AutoNSubstituteData]

Suggestion

disable this rule in test / .editorconfig with dotnet_diagnostic.SA1133.severity = none

Unless a rule is very unambiguous what it should be set to, I tend to think that it should be set to "info" rather than "warn" or "error". In many of the rule sets there are good suggestions for how to write code, but I fear our code will be littered with suppressions. The types of rules that typically fall into this section is correctness rules and performance rules, that are not always relevant in the context.

StyleCop rules on the other hand, which are all about consistency, and not about e.g. correctness or performance, them we should agree on and try to follow as much as possible.

Rule summary

What is the problem

This does not improve readability as the rules description claims.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.MA0006.severity = none

Rule summary

What is the problem

I completely agree with this rule for code projects!

However, for test projects, it makes very good sense to use underscores in method names to express the purpose of a unit test.

• Should_Return_Subscription_From_Repository
• Should_Return_NotFoundResponse_When_Subscription_Does_Not_Exist

This provides a better readable context understanding.

Suggestion

disable this rule in test / .editorconfig with dotnet_diagnostic.CA1707.severity = none

Rule summary

What is the problem

Forcing always to have parameter documentation added is a waste of time.

Suggestion

Set it to none or info.

Rule summary

What is the problem

No. It must not always end with a period. Sometimes you end with a code snippet, or other text where a period doesnt make sense.

A better alternative is to install a plugin like https://marketplace.visualstudio.com/items?itemName=EWoodruff.VisualStudioSpellCheckerVS2017andLater that helps with spelling in code and VS.

Suggestion

none

The name of our tests is one of the most important parts of writing tests, thus, I think it would be good to have a general convention for how we write the names of our tests.

My own preference is to be as wordy as needed, but no more than needed, and follow the given - when - then pattern, mostly without using those words, e.g. for a test of a simple add method on a calculator: add called with a and b results in a plus b (this would perhaps be a theory).

Rule summary

What is the problem

I don't like this rule... because the use of string interpolation often provides better readability, and the performance problem here is in the compiler - and the compiler needs to be corrected instead.
Until the compiler is fixed, it is not a issue in real world scenarios.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.MA0028.severity = none

Rule summary

What is the problem

The first time I saw this rule I did not like it and would suggest it should be disables, but after reading up on the rule, I will now say it is very relevant with the arguments for readability.

Therefor: Just follow the rule - see 'How to fix violations'

Suggestion

enable this rule in root .editorconfig with dotnet_diagnostic.MS0003.severity = error

According to the docs at https://github.com/dotnet/roslyn-analyzers, the NetAnalyzers is not needed when we use the .NET5 SDK or later in our projects. As far as my understanding goes, these analyzers will be included just by having .NET5 SDK installed and using that to build with, even if the project we are building has TargetFramework < NET5.

You do not need to manually install this NuGet package to your project if you are using .NET5 SDK or later. These analyzers are enabled by default for projects targeting .NET5 or later. - https://github.com/dotnet/roslyn-analyzers#microsoftcodeanalysisnetanalyzers

Rule summary

What is the problem

This rule sounds very nice as a starting point.
But in practice, there are many situations where it can not be done due to nested method call or general formatting for space optimization. Furthermore, after C # 8 with the nullable suppression operator !, this rule creates a lot of noise.

Example 1:

var myData = JsonSerializer.Deserialize<TEntity>(data!.ToString()!);

Then the rule says .ToString()! need a space like this .ToString() !. And this space is removed by the default Visual Studio's document formatter. And the use of the nullable suppression operator need to stick close to the type.

Example 2:

.ReturnsForAnyArgs((Entities.User?)null)

Then the rule says .ReturnsForAnyArgs((Entities.User?)null) need a space like this .ReturnsForAnyArgs((Entities.User?) null).

Furthermore, a solution with SuppressMessage attribute is overkill, therefore I recommend to suppress this rule since is obsolete in relation to C # 8 with nullable check activated.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1009.severity = none

Here is a link to all the relevant rules: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/DocumentationRules.md

Rule summary

What is the problem

List<T> or IList<T>

In a theoretical world, it sounds good to always use interfaces over concrete implementation. But this rule does not seem very good in practice and with reference to this post https://stackoverflow.com/questions/400135/listt-or-ilistt
with answers to the same questions by Arec Barrwin, I will suggest the general rule should be just use List<T> and if you want to over-engineering then feel free to use IList<T>.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.MS0016.severity = none

Rule summary

What is the problem

We name files after a different pattern. Example: CommandFixture<TParameters, TResult> = CommandFixture{TParameters,TResult}.cs

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.MA0048.severity = none

Rule summary

What is the problem

We are missing a decision compared to SA1600, then they should be aligned.

Suggestion

Should be aligned with SA1600

Rule summary

What is the problem

When writing tests and using e.g. NSubsitute to verify that a call has happened, you often have to fake calling it to create the assertion. If the method we are asserting was called/not called returns a value, this rule will complain that we have a returned value that needs to be assigned to either a variable or a discard (_).

Suggestion

[tests/**.cs]
dotnet_diagnostic.IDE0058.severity = silent         # IDE0058: Expression value is never used

Rule summary

What is the problem

MA0038 is a duplicate of MS CA1822

And CA1822 should be set as info since it is a Performance

Suggestion

enable this rule in root .editorconfig with dotnet_diagnostic.CA1822.severity = info
disable this rule in root .editorconfig with dotnet_diagnostic.MA0038.severity = none

Rule summary

What is the problem

The rule goes against a common practice advocated by Microsoft in samples etc. for as long as I can remember.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1201.severity = none

Rule summary

What is the problem

This rule flags unused parameters. The option value determines if unused parameters should be flagged only for non-public methods or for both public and non-public methods.

Suggestion

Should be set to error in ROOT

Rule summary

What is the problem

Test assemblies that are generated by ATC generator can cause this.

Suggestion

How to handle this for test code, is this something that makes sense to suppress?

I think it would make sense to at least start a conversation about potential sematic versioning of these rules.
The main purpose would be to convey information to users of the rules about the "severity" of running an update.

This is not to say that it is an urgent priority at all. I just had the thought today and a few minutes to type this out, so I figured it was better to document it here and open the discussion, or at least save it for later :-)

From a high level perspective, I see the following levels of changes/impact, and we should maybe discuss if a) I missed something important, and b) how it makes most sense to map these to a versioning scheme (preferably SemVer).

1. Minor functional changes that don't impact number of rule sets or severity of any rules at all.
2. Rule changes that only make the rules more lenient, i.e. disable previously existing rules. I.e. changes that should not catch anything on existing code that builds 🟢, but would potentially mean that you might have to/want to remove no longer needed exclusions in your custom section(s).
3. Rule changes that increase severity on one or more rules, e.g. a change where I as a project maintainer must expect to either do some cleanup work now or add new custom overrides until we have the time to fix.
4. Dependency updates to existing rule sets. These might potentially have either less or more "impact" than changing the default ATC rule settings. An update could be adding a bunch of new rules, or it just be a bugfix to an existing rule.
5. Adding a whole new rule set. I.e. expect a lot of work or at least a lot of new custom excludes.
6. Breaking changes that change e.g. the directory layout or other things (like the recent case-changing of props files. Breaking changes like this more or less require the "user" to read some sort of migration guide (or have access to internal knowledge).

Do we want this Analyzer added?

https://www.nuget.org/packages/NSubstitute.Analyzers.CSharp/

If so - we need to add the following to our distribution/build/code-analysis.props file.

<PackageReference Include="NSubstitute.Analyzers.CSharp" Version="1.0.14" />

Rule summary

What is the problem

Sometimes the parameter makes enough sense based on the context they are used in, making a <summary> tag enough to document a public API. Forcing always to have parameter documentation added is a waste of time.

Suggestion

Set it to none or info.

Rule summary

What is the problem

Trailing comma or not?

I (Egil) wonder why we force a trailing comma after the last property when using object initializers, for example:

new Pagination<ContractRecipe>
{
    PageSize = 50,
}

vs.

new Pagination<ContractRecipe>
{
    PageSize = 50
}

or for larger initializsers:

new QueryOptions
{
    PageSize = parameters.Request.PageSize,
    OnlyExactTagMatch = parameters.Request.OnlyExactTagMatch,
    OnlyMasterRecipes = parameters.Request.OnlyMasterRecipes,
    ContinuationToken = parameters.Request.ContinuationToken,
    Tags = parameters.Request.Tags.ToDomainType(),
}

vs.

new QueryOptions
{
    PageSize = parameters.Request.PageSize,
    OnlyExactTagMatch = parameters.Request.OnlyExactTagMatch,
    OnlyMasterRecipes = parameters.Request.OnlyMasterRecipes,
    ContinuationToken = parameters.Request.ContinuationToken,
    Tags = parameters.Request.Tags.ToDomainType()
}

It seems unnatural to me. It seems inconsistent with e.g. how method and constructor calls work, where a trailing comma is not allowed.

Microsofts reasoning relates to being able to blame others in git history:
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1413.md

Rule summary

What is the problem

Rule should dictate how we document code.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1623.severity = none

Rule summary

What is the problem

This rule is related to the main rule SA1600 with issue #5

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1604.severity = none

Rule summary

What is the problem

This rule only seems relevant if you want to expose a documentation page / system generated by a tool.
But for readable code and no needs for code documentation, this rule provides no value but only noise.
If it was necessary for a project to have code documentation, it would be better to only enable it for the specific project need in the SRC/[project folder]/.editorconfig.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1600.severity = none

Rule summary

What is the problem

When writing tests/theories that receive their input through arguments generated by e.g. AutoFixture, it doesnt always make sense to have the CancellationToken as the last argument.

Suggestion

TEST = none

Rule summary

What is the problem

This is a rule that is more about religion.
But as a starting point, it does not provide real value or increase the readability of the code.
However, it provides more clarity when inheriting / overwriting where this and base should be used, and only when needed.
Furthermore, it has not always been the standard that Microsoft has used or is doing now.

Note: By default, StyleCop disallows the use of underscores or m_ to mark local class fields, but that doesn't means this should be used instead.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1101.severity = none

Rule summary

What is the problem

The documentation text within a C# property's

We want to update Meziantou.Analyzer to v. 1.0.645.

In order to do this, we need all downstream Github/AzureDevOps pipelines to implement dotnet restore steps differently, otherwise these pipelines will fail.

Rule summary

What is the problem

I work primarily with projects that do not have a SynchronizationContext, therefore this rule is not usable I think.
And very annoying to have to type .ConfigureAwait(false) everywhere if not needed.
But am not quite sure after reading these 2 blog post:

• https://blog.stephencleary.com/2017/03/aspnetcore-synchronization-context.html
• https://devblogs.microsoft.com/dotnet/configureawait-faq/#ive-heard-configureawaitfalse-is-no-longer-necessary-in-net-core-true

But most certainly, this rule does not make sense at all in test projects.

Note: this rule is the same same as MA0004

Suggestion

I am not sure, but think it should be removed/suppressed from test projects at least.

Rule summary

What is the problem

We name EndpointFixture<TRequest, TResource> as EndpointFixture{TRequest,TResponse} - the rule dowsn't allow it.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1649.severity = none

Rule summary

What is the problem

The .NET Guidelines for async methods includes that such methods should have names that include an "Async" suffix.

Methods that return awaitable types such as Task or ValueTask should have an Async suffix. Methods that do not return awaitable types should not use the Async suffix.

This should not be necessary for test projects - since a test methods name is describing the intended functionality and not async structure.

Suggestion

disable this rule in test / .editorconfig with dotnet_diagnostic.VSTHRD200.severity = none

Rule summary

What is the problem

This rule is related to the main rule SA1600 with issue #5

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1602.severity = none

Align Directory.Build.props with Atc.Cosmos repo configuration.

Rule summary

What is the problem

Duplicate warnings are generated for the same issue.

Suggestion

Disable MA0011 and prefer the Microsoft rule.

If we include the following property <SymbolPackageFormat>snupkg</SymbolPackageFormat> , the dotnet pack fails with the following error:

C:\Program Files\dotnet\sdk\5.0.102\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(207,5): error NU5017: Cannot create a package that has no dependencies nor content. [C:\Code\atc-net\atc-coding-rules-updater\src\Atc.CodingRules.Updater.CLI\Atc.CodingRules.Updater.CLI.csproj]

This seems to work in a class library, but apparently not in a CLI tool.

Reference issue

Rule summary

What is the problem

This rule only seems relevant if you want to apply copyright or the like to all your code files.
But in a world with more open source or the desire for more readable code that is not muted with commentary as copyright.
Then this rule is seen as something that gives no value but only noise.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1633.severity = none

We have never used this analyzer before, what are the reasons behind adding it?

Rule summary

csharp_style_prefer_index_operator and csharp_style_prefer_index_operator is set today as 'true'

What is the problem

The suggestion is not improving the readability, in some cases make it worse.

IDE0056 example:

// csharp_style_prefer_index_operator = true
string[] names = { "Archimedes", "Pythagoras", "Euclid" };
var index = names[^1];

// csharp_style_prefer_index_operator = false
string[] names = { "Archimedes", "Pythagoras", "Euclid" };
var index = names[names.Length - 1];

IDE0057 example:

// csharp_style_prefer_range_operator = true
string sentence = "the quick brown fox";
var sub = sentence[0..^4];

// csharp_style_prefer_range_operator = false
string sentence = "the quick brown fox";
var sub = sentence.Substring(0, sentence.Length - 4);

Suggestion

Change:
csharp_style_prefer_index_operator = true => false # IDE0056
csharp_style_prefer_range_operator = true => false # IDE0057

The two rules are the same. This I think we should prefer the one from Microsoft and simply disable MA0016, and replace the doc/decision file (#12) for it with one for CA1002.

Rule summary

What is the problem

This rule sounds very nice as a starting point.
But in practice, we offen need // TODO: and NotImplementedExecption when we scaffold method-body's, and will do the actual implementation later of just handover to a co-worker.

S1135 is about TODO's

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.MS0025.severity = suggestion

Rule summary

What is the problem

This rule flags classes that are not instantiated in a library and are internal as not being used. In the docs for the rule, it says it is safe to suppress the rule if the classes is instantiated through other means, e.g. an IOC container.

One example is when we have an public interface (IMyEndpoint) and an internal implementation (MyEndpoint) that is registered in an IServiceCollection through an extension method. In this case, the rule still gives a warning, even though the class is actually used.

Suggestion

ROOT: NONE.

Rule summary

What is the problem

When writing tests and using e.g. NSubsitute to verify that a call has happened, you often have to fake calling it to create the assertion. If the method we are asserting was called/not called is an Async method, this rule will complain that the call was not awaited.

Suggestion

[tests/**.cs]
dotnet_diagnostic.CS4014.severity = suggestion      # CS4014: Because this call is not awaited, execution of the current method continues before the call is completed

Rule summary

What is the problem

This rule have a conflict This rule have a conflict with root .editorconfig csharp_using_directive_placement = outside_namespace

And there is strong opinions on the internet about this rule - but also a fact about assembly loading issues which should be taken into consideration which should be taken into consideration
Read Scott Hanselman post

And default create new class in Visual Studio have alway used outside_namespace which means much written code over time has been followed the outside_namespace rule.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1200.severity = none

Rule summary

What is the problem

As a "Readability" rule, I would claim that exchanging "" with String.Empty actually decreases readability in many places.
Once upon a time, in ancient .Net 1.x days, there were optimisation reasons for using String.Empty as "" would instantiate a new String constant for every use, but to my knowledge this has long passed.

Also, there are quite a few places in CSharp where String.Empty is actually not a legal replacement for "" AFAIK. (like switch/case statements), so forcing people to learn this habit that can then not even be universally enforces seems plain wrong.

The only real downside of "" that I can think of, is the off chance that it can hide zero-width characters, but a) who has ever had that happen in the wild? , and b) if this is a problem, there should probably be a rule specifically for this case, forcing devs to write a very explicit local rule override if they ever needed to out a "<zero-width>xN" string in the code on purpose.

Suggestion

Suppress this rule in the standard ATC rule set.

Rule summary

What is the problem

I do not see ordering of static / non-static methods as an issue and some freedom should be allowed.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.SA1204.severity = none

Remove PackageReference to Microsoft.CodeAnalysis.NetAnalyzers since we are already enforced to use NET 5 as build SDK and have EnableNETAnalyzers set to true

SA1202 should be covered by decision for SA1201 (#23).