Closed elachlan closed 1 year ago
niemyjski
commented
2 years ago @ejsmith can you take a look at this?
@elachlan Thanks for the PRs! Thinking out loud, I'm fine with bringing in settings but I'd be more inclined to pick up ones by the Rosyln team and then just apply code formatting via dotnet format against the whole project. Out of all the rules I'm a bit hesitant to turn rules off.
elachlan
commented
2 years ago @ejsmith can you take a look at this?
@elachlan Thanks for the PRs! Thinking out loud, I'm fine with bringing in settings but I'd be more inclined to pick up ones by the Rosyln team and then just apply code formatting via dotnet format against the whole project. Out of all the rules I'm a bit hesitant to turn rules off.
Roslyn's team I think does a lot of custom analyzers. The runtime repo has the best set of rules, which is what I used to base the msbuild rules off of. The msbuild team then turned off rules that they didn't need, felt were not helpful, or the cost benefit didn't weigh up.
The idea behind this change is that any violations show up in the IDE and build system when PRs are submitted.
elachlan
commented
2 years ago Removing the disabled rules causes 1800 build errors. Most of those rules are probably best left off unless there is a specific reason.
ejsmith
commented
2 years ago So I guess this is a bunch of additional rules in addition to the ones we are using in Exceptionless/Foundatio? https://github.com/FoundatioFx/Foundatio/blob/master/.editorconfig
I'm definitely for having linting rules and even enforcing them to ensure consistency, but I just don't want to have inconsistent rules between our projects / repos. Also, I'm not willing to have it spit out 1000s of warnings and make our build output horrible. Ideally, we would have an enforced set of rules and then get our code to conform to those rules, but we just don't have the bandwidth to go cleanup everything right now.
ejsmith
commented
2 years ago I don't see all of those rules in the dotnet/runtime repo: https://github.com/dotnet/runtime/blob/main/.editorconfig
This really depends on the core team on exceptionless and what rules they want to keep. I have pulled in the rules used by MSBuild which seemed reasonable. https://github.com/dotnet/msbuild/pull/5656
Generally with a change like this, the analysers are set to suggestions and moved to warnings or error once all instances are fixed. Each code analysis rule would be fixed in an separate PR. So this PR should be used to establish a list of rules to work against.