search

dotnet / roslyn-analyzers

MIT License
1.58k stars 465 forks source link

Add StyleCop analyzer on this repository #3744

Open Evangelink opened 4 years ago

Evangelink commented 4 years ago

I think it could be valuable to activate StyleCop Analyzer on this repository and to define some styling guidelines.

One of the example, could be the rule about having a message for a Debug.Assert. Recently when compiling locally I have a lot of assertions failing (I have tried to run git clean -dfx without much success) but there is no message displayed so it's hard to see if there is any need to report some issue. I do know and see that they don't seem to fail on the CI as the build is green.

Evangelink commented 4 years ago

Tagging @sharwell and @mavasani here.

sharwell commented 4 years ago

... having a message for a Debug.Assert

https://github.com/dotnet/csharplang/issues/287 would be a big help for this.

Evangelink commented 4 years ago

StyleCop could still be useful for putting some styling standards in place in the codebase and could prevent some of the "Un-needed blank line" review comments :)

sharwell commented 4 years ago

could prevent some of the "Un-needed blank line" review comments :)

IDE0055 (formatting analyzer) covers most of the configured style. For blank lines, RS0101 is implemented in the Roslyn.Diagnostics.Analyzers package and could be enabled. Prior to RS0101 getting enabled, there should not be any comments in code review regarding blank lines. If we have an analyzer but do not turn it on, then the style in question is no longer eligible for comment in code review, and/or comments regarding it can be marked resolved without taking any other action.

Evangelink commented 3 years ago

I'd like to revive this subject with a new example. Although I totally agree with @sharwell when saying that styling/formatting comments should not block a merge if there is no analyzer enforcing its application, I think that there is some implicit consensus around the fact that it's nice to have an empty line between a closing brace and some new statement (except another closing brace). Looking at the various rules available with Microsoft.CodeStyle analyzers I think there is nothing to enforce it and so this could be nice to either add a new rule for it in the analyzer or to use the one from StyleCop. WDYT?

Evangelink commented 3 years ago

ping @mavasani @sharwell

mavasani commented 3 years ago

I will let @sharwell drive this. I have no pushback on this, but I believe Sam received some pushback in past on similar work on Roslyn repo.