Skip to content

Conversation

@elachlan
Copy link
Contributor

@elachlan elachlan commented May 2, 2022

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.
dotnet/msbuild#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.

@niemyjski niemyjski requested review from ejsmith and niemyjski and removed request for niemyjski May 3, 2022 01:07
@niemyjski
Copy link
Member

niemyjski commented May 3, 2022

@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
Copy link
Contributor Author

elachlan commented May 3, 2022

@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
Copy link
Contributor Author

elachlan commented May 5, 2022

Removing the disabled rules causes 1800 build errors. Most of those rules are probably best left off unless there is a specific reason.

@ejsmith
Copy link
Member

ejsmith commented May 10, 2022

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
Copy link
Member

ejsmith commented May 10, 2022

I don't see all of those rules in the dotnet/runtime repo: https://github.com/dotnet/runtime/blob/main/.editorconfig

@elachlan elachlan closed this Jan 27, 2023
@elachlan elachlan deleted the implement-helpful-code-analysis-rules branch January 27, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants