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

Microsoft Code Analysis #1781

Open
amoerie opened this issue May 2, 2024 · 2 comments · May be fixed by #1782
Open

Microsoft Code Analysis #1781

amoerie opened this issue May 2, 2024 · 2 comments · May be fixed by #1782

Comments

@amoerie
Copy link
Collaborator

amoerie commented May 2, 2024

What is, according to you, missing from fo-dicom? Please describe.
We currently don't have any static code analysis enabled, except for the compiler itself.
Since .NET 5, Microsoft provides an excellent set of Roslyn Analyzers that are enabled automatically if you target .NET 5 or higher.
Since we target .NET Standard 2.0, they must be enabled manually.

One example of things they catch is missing "ConfigureAwait(false)" calls. It looks like #1776 was mostly caused by that.
While @gofal's PR fixes it this time, having these analyzers would prevent the same problem from happening again.

Describe the solution you'd like
I would like to enable these analyzers and fix the issues that arise.

Describe possible alternatives or existing workarounds you've considered
There are external tools like SonarCloud, but those are much more elaborate (read: would require much more time to fix all issues) and usually cost money. The Roslyn Analyzers are free, high quality and real time.

Link: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/overview?tabs=net-8

@amoerie amoerie linked a pull request May 2, 2024 that will close this issue
5 tasks
@gofal
Copy link
Contributor

gofal commented May 5, 2024

Of course it is a good idea to enable code analysis.

But still, the code analysis can warn or give suggestions. Often the warnings are a good idea, but sometimes they are false warnings. Meaning, we should not blindly trust the code analysis like lemmings.
If working on a issue and testing it, then the analysis and warnings of this code should be fixed or evaluated by the programmer.

But we should not turn on code analysis, and then also turn on TreatWarningsAsError, which then forcues us to change all the code across all the project.

@amoerie
Copy link
Collaborator Author

amoerie commented May 6, 2024

Of course it is a good idea to enable code analysis.

But still, the code analysis can warn or give suggestions. Often the warnings are a good idea, but sometimes they are false warnings. Meaning, we should not blindly trust the code analysis like lemmings. If working on a issue and testing it, then the analysis and warnings of this code should be fixed or evaluated by the programmer.

But we should not turn on code analysis, and then also turn on TreatWarningsAsError, which then forcues us to change all the code across all the project.

In general my experience with Microsoft.CodeAnalysis is that they are quite conservative, and safe to apply. If we disagree with a specific warning, or if it would introduce too much work, we can disable them (and I have already done so a few times in my PR).

You can configure the severity level of each rule separately in .editorconfig. I have set everything in the "reliability" and "performance" category to "Warning" for now, since these are important aspects of a library. For example, "ConfigureAwait" is only a suggestion by default, but as you have experienced, they can cause quite some trouble.
The other rules are still set to their default values.

If you have some time, please look at some of the changes I made with the currently activated set of rules. Please say so if disagree with some of them. I made separate commits per rule, so it is just a matter of reverting the commit and changing the severity of the rule.

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

Successfully merging a pull request may close this issue.

2 participants