atc-net / atc-coding-rules-updater Goto Github PK

I was running version 2.0.82.
First off, very happy to see that it now correctly reposts the right version with --version and also notifies me about an available update.

But it seems that it is still slightly confused about what is available.

$ atcu --sanity-check .
  ____            _                                      _           _
 |  _ \   _   _  | |   ___   ___     _   _   _ __     __| |   __ _  | |_    ___   _ __ 
 | |_) | | | | | | |  / _ \ / __|   | | | | | '_ \   / _` |  / _` | | __|  / _ \ | '__|
 |  _ <  | |_| | | | |  __/ \__ \   | |_| | | |_) | | (_| | | (_| | | |_  |  __/ | |   
 |_| \_\  \__,_| |_|  \___| |___/    \__,_| | .__/   \__,_|  \__,_|  \__|  \___| |_|   
                                            |_|
Version 2.0.88 of ATC-Coding-Rules-Updater is available!
You are running version 2.0.82.41834

finds new version 2.0.88 but then immediately updates to 2.0.92 ;-)

$ dotnet tool update --global atc-coding-rules-updater
Tool 'atc-coding-rules-updater' was successfully updated from version '2.0.82' to version '2.0.92'.

Instead of failing hard and not writing anything on errors, the caching mechanism should instead rely on one file per provider, and thus be able to cache collected rules for all non-failing rule-sets.

Problem: I have a few projects where the coding-rules-updater can't complete the Release builds needed to generate the temporary suppressions, e.g. because the Release build runs a code obfuscator that causes various namespace/using related errors.
I can manually comment out the import line in the csproj file, run the atc-coding-rules-updater run -s . and then add the line back, but this is not a nice workflow when doing it manually, and also makes automation near impossible (or at least very ugly and failure prone).

I don't know exactly what the right solution is here as I am not very knowledgeable in dotnet build, but it would be really nice if it would be possible to send a parameter to the updater to skip certain named build targets when running the repeated build for generating temporary suppressions.

I am happy to assist where I can with more information, or with testing possible solutions.

I don't know if it is possible, but it would be really nice if the options-file create command didn't add a section for a sample folder if none is present in the existing project.
If this is not a realistic feature to implement, then we should probably output a warning to check/validate the content of the generated json file and remind the user that the generator is not that intelligent ;-)

As a user running the create command, it is hard to know if the generator actually analyses my project or just outputs a dummy json file that happens to nearly match my paths.

I had a new project that needed coding-rules added. This project did not have the default src/ and test/ folders, but rather ProjectName and ProjectNameTest subfolders, so I started by running atc-coding-rules-updater options-file create . to create a default atc-coding-rules-updater.json file, and then edited the paths in this one.

Afterwards, I ran the run subcommand to add all the coding-rules stuff, and this ran perfectly smooth. However, the actual output says:

Since the paths in the output are formatted to look like actual paths src/.editorconfig then maybe it would be less confusing if they actually printed the correct path to the file that was created e.g. ProjectNameTest/.editorconfig rather than the "conceptual" test/ path?

Maybe there is a good reason why it does what it currently does?

It looks as if having the

    <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>

set in your csproj file makes dotnet build enforce ALL rules, even if they are disabled in .editorconfig.

This may need more investigation.

Removing the line solves the problem, but this has to be at least documented, probably addressed somehow in the atc-coding-rules-updater (either autofix or warn about it).

To reproduce:

$ atc-coding-rules-updater.exe --version
1.0.0.0

To make sure that the following custom rules work as intended, even if the user has added other custom rules for instance for overriding normal editorconfig settings, we should specify the *.cs pattern before the rules.

This should be included in all .editorconfig files generated, and maybe in both the normal Custom section and the temporary suppression section.

If the project has:

 <EnableNETAnalyzers>true</EnableNETAnalyzers>

set already, then running the atc-rules-updater to add rules etc. will cause the project to fail with something like:

CSC : error CS8034: Unable to load Analyzer assembly C:\Users\extjkrag\.nuget\packages\microsoft.codeanalysis.netanalyzers\5.0.3\analyzers\dotnet\cs\Microsoft.CodeAnalysis.CSharp.NetAnalyzers.dll : Assembly with same name is already loaded [C:\Users\extjkrag\source\repos\XYZ\XYX.Test\XYX.Test.csproj]
CSC : error CS8034: Unable to load Analyzer assembly C:\Users\extjkrag\.nuget\packages\microsoft.codeanalysis.netanalyzers\5.0.3\analyzers\dotnet\cs\Microsoft.CodeAnalysis.NetAnalyzers.dll : Assembly with same name is already loaded [C:\Users\extjkrag\source\repos\XYX\XYX.Test\XYX.Test.csproj]

Removing the EnableNETAnalyzers declaration so that it is only included in Directory.build.props fixes this issue, but this is not very obvious to end users, and maybe the updater cli should handle this in some nice way?

As I understand it, the general recommendation when starting with ATC rules on an existing project, is to initially suppress all failing rules in your Custom section in order to get back to ✅ state for the project, and then later clean up the code and remove the suppressions one by one in nice clean commits.

The challenge is that this initial suppression list takes a long time to hand-write. There is no easily translatable output from dotnet build that you can use unless you do serious amounts of manipulation (sorting, cleaning format etc.) in an editor or spread sheet to get the right format.

It would be really nice if we could somehow generate this list for the user. I am not really a .Net guy, so I don't know how this could be plugged into the ecosystem (as it is only dotnet build that knows which rules are violating it is probably not a good job for the atc updater to do this).

I envision the tool generating the actual syntax needed for insertion in the .editorconfig files, or even auto-adding to the custom sections. It would be a bonus if rues that only fail in test/ are only excluded in that folder, so as not to "cloak" future errors in production code.

Ideally, I would wish for a format where each line also contains information about the rule, e.g. the description and maybe link:

# StyleCop
# https://github.com/DotNetAnalyzers/StyleCopAnalyzers
dotnet_diagnostic.SA1309.severity = none    # https://documentation.help/StyleCop/SA1309.html FieldNamesMustNotBeginWithUnderscore
dotnet_diagnostic.SA1402.severity = none    # https://documentation.help/StyleCop/SA1402.html FileMayOnlyContainASingleClass

# SonarSource
dotnet_diagnostic.S101.severity = none      # https://rules.sonarsource.com/csharp/RSPEC-101 Types should be named in PascalCase
dotnet_diagnostic.S108.severity = none      # https://rules.sonarsource.com/csharp/RSPEC-108 Nested blocks of code should not be left empty

# https://cezarypiatek.github.io/post/non-nullable-references-in-dotnet-core/
dotnet_diagnostic.CS8600.severity = none    # Converting null literal or possible null value to non-nullable type.
dotnet_diagnostic.CS8601.severity = none    # Possible null reference assignment.

# Meziantou
# https://www.meziantou.net/enforcing-asynchronous-code-good-practices-using-a-roslyn-analyzer.htm
dotnet_diagnostic.MA0002.severity = none    # https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0002.md IEqualityComparer<string> or IComparer<string> is missing
dotnet_diagnostic.MA0004.severity = none    # https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0004.md Use Task.ConfigureAwait(false)

The example above is a very shortened snippet from the hand-crafted exclusion list I made when adding ATC rules to a small Swagger API project. The full list originally contained > 100 rule suppressions, and adding the links and comments took quite a few hours, but I judged that it would be so much easier to motivate the developers to clean up the rules if they could actually read which one they were commenting out instead of just going by trial and error and having VS do useless Bing searches for help.

To me this sounds like a perfect use-case for automation.

To help users have the latest version of the updater tool installed, the tool should query nuget.org/look at github releases and see if it has the latest version installed.

This could probably be done by checking the the version timestamp in the asesmbly and compare it to the latest released to nuget.org.

If there is a newer version, the tools should output the approprate dotnet command to update the tool.

This is not (yet) a specific feature request, but more an attempt to start a discussion about this general topic.

The updater is really cool and smart, but at the moment it is very dependent on developers remembering (and being motivated) to run it once in a while. If you have many projects using the atc-coding-rules, I expect it will become a huge, boring and then quickly forgotten task to actually keep the rules up to date.

Just like with other similar projects, this should be solvable with automation, e.g. in a pipeline.

As a MVP, the updater should have sort of a dry-run run-mode (switch) that just reports a zero or non-zero exit code depending on whether there are actually any updates. This would at least allow setting up a daily/weekly pipeline job that just fails if there are any pending updates, thus (hopefully) alerting someone on the team to manually run the updater and "do what needs to be done".

A more advanced solution could be something in the style of Dependabot that actually creates and submits a Pull Request with the updates. This would then automatically be built and can be reviewed/merged by a developer. The challenge with this is that it becomes platform dependent as there is no common interface for PR's across different Git hosting systems.
On the other hand, this could probably be done quite easily in simple pipeline steps on most platforms, and might therefore not (yet) be in reasonable scope for ATC?
I.e. If I can run the updater in a pipeline and easily "detect" if there are any real changes, then I can also get my pipeline to create a branch and make a PR.

So for now, the main point for me is to figure out which features are needed in the updater tool to better support automation.

I ran the updater today, and the output made me indirectly realize that my internet connection was completely down. The updaters error message about "no such host is known" are correct, but don't really tell the user what it was trying to fetch.
Also, in the end, it just continues "silently" to the .editorconfig / temp. suppressions part, and ends up reporting a perfectly clean green "Done" without any messages about the possible side effects of the previous sections not running correctly.
In my case, it was all on one screen, and I noticed the warnings, but if there had been more suppression output, I might have missed the initial red lines.

I am not sure what would be the best behavior here, but figured I would post the screenshot while I had it, for further discussion.

When running atc-coding-rules-updater options-file create . it always generates an options json with target set to DotNet6, even on a DotNet5 project.
If we can't realistically autodetect the framework, then it would be nice if this subcommand at least could take the same -t option as the regular run command.

If the user wants to override the default editor rules (e.g. whitespace) this can currently only really be done in the Custom section for coding rules, but since this involves specifying new file-extension wildcards, it may cause al following rules to be ignored for cs files.

This is closely related to issue #44

When running the updater with --useTemporarySuppresions, it adds lines to .editorconfig for each violating rule. The comment generated for each violation includes the number of occurrences found. This is quite useful when triaging which problems are easy to attack and clean up.
I have however seen a few examples where the updater vastly underestimates the number of violations. E.g. my .editorconfig after running mentioned "# occurrences 13 ..." for SA1503, but when I commented out the suppression, and rana build, there were way over 50 errors.

I am running on a solution with 5 projects, so maybe the updater is just counting the occurrences in the first project it finds, generating the exclusion, and then somehow not adding to the number for later projects analysed? (pure guess)

This is not a critical issue by any means, but worth looking at if this part of the code is revisited for other reasons.
If the generator bothers to mention the number of occurrences, it does look better if it is actually correct 😄

When running the updater with temporary suppressions, it creates a whole comment block that includes timestamp of the run:

##########################################
# ATC temporary suppressions
# generated @ Thursday, January 27, 2022 10:35:50 PM
# Please fix all generated temporary suppressions
# either by code changes or move the
# suppressions one by one to the relevant
# 'Custom - Code Analyzers Rules' section.
##########################################

I was wondering if it would be useful for this block to also contain a line about the version of the updater used to generate this?
E.g. Generated by: atc-coding-rules-updater v. 2.0.92

The updater rereferences package Meziantou.Analyzer in version 1.0.638 which is not supported by Devops pipelines.

When running the command, it will print out a duplicate key warning. This may be because the same key exists twice with different values or with the same value, and I have no way of seeing which.
In case of a duplicate key, it would be nice if it printet out both/all the duplicates including the set value, preferably with a line number so they are easy to find in the file.

If might be good if this extra output was covered by either a verbose switch to enable, or a quiet switch to silence it?