For years, no one has dealt with the warnings that have been accumulating as .NET code analysis have matured.
Results
Before
There were hundreds of warnings that were being ignored.
After
These have all been addressed, and each project has had "Treat warnings as errors" turned on.
There was a lot to get through, so in many cases, the simplest solution was often taken (e.g. using "!" or "?" etc.)
But this is more about helping future developers, and less about fixing all the issues that already exist.
Some places it was easier to suppress the warning than solve it, as the difference between .NET4.8 and .NET6.0 made it harder to solve.
With models, I tried to reflect whether a property was legitimately nullable or not.
AssertAsync
We had to add an overload that was not async (i.e. sync), which made this class name not make sense anymore. So we renamed to AssertException.
Also, whenever we used the async version, and gave it a task from a Task.Run, it would warn about this. So we made the overload that just takes the task itself, and suppress the warning in that overload.
.editorconfig
This was added so we could disable VSTHRD200. This was a nightmare in the test project, as it wanted all our async tests to end with the word 'Async'. But this isn't a rule we are respecting in Halibut itself, so it was disabled.
How to review this PR
There is a lot here.
To help break it up, each commit attempts to achieve something specific towards the final goal.
Please review each commit (or the whole thing if that works better for you).
As you review, please try to see if I have converted something incorrectly.
Otherwise, Quality :heavy_check_mark:
Pre-requisites
[ ] I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
[ ] I have considered informing or consulting the right people, according to the ownership map.
[ ] I have considered appropriate testing for my change.
Background
For years, no one has dealt with the warnings that have been accumulating as .NET code analysis have matured.
Results
Before
There were hundreds of warnings that were being ignored.
After
These have all been addressed, and each project has had "Treat warnings as errors" turned on.
There was a lot to get through, so in many cases, the simplest solution was often taken (e.g. using "!" or "?" etc.)
But this is more about helping future developers, and less about fixing all the issues that already exist.
Some places it was easier to suppress the warning than solve it, as the difference between .NET4.8 and .NET6.0 made it harder to solve.
With models, I tried to reflect whether a property was legitimately nullable or not.
AssertAsync
We had to add an overload that was not async (i.e. sync), which made this class name not make sense anymore. So we renamed to
AssertException.Also, whenever we used the async version, and gave it a task from a
Task.Run, it would warn about this. So we made the overload that just takes the task itself, and suppress the warning in that overload..editorconfig
This was added so we could disable VSTHRD200. This was a nightmare in the test project, as it wanted all our async tests to end with the word 'Async'. But this isn't a rule we are respecting in Halibut itself, so it was disabled.
How to review this PR
There is a lot here.
To help break it up, each commit attempts to achieve something specific towards the final goal.
Please review each commit (or the whole thing if that works better for you).
As you review, please try to see if I have converted something incorrectly.
Otherwise, Quality :heavy_check_mark:
Pre-requisites