Opentelemetry-dotnet: Reduce StyleCop terseness

[austinlparker]

There are concerns about the amount of violations stylecop finds - we should turn it down.

[enzian]

What rules are you referring to?

[austinlparker]

@lmolkova could maybe give some more insight as she brought this up in the weekly meeting, but I think the major complaints were around nitpicky sort of style things that should theoretically be automatically fixed (like trailing commas or newlines)?

[enzian]

that should theoretically be automatically fixed

I agree but fact is: They are were/are not. I fixed a few thousand warnings where a good part were whitespacing issues that also lead to bad readability and thank god for the ability to apply fixes to solutions...

The thing is, while it may be nitpicking - disabling rules will lead to them not being followed and no one noticing it in reviews.

[discostu105]

Just my personal opinion: I'm ok with strict style rules. It avoids errors that would need follow up commits, just to fix styling.

What I am slightly annoyed by, is the "every methods needs XML documentation" rule. I am absolutely FOR having clear and good documentation on the most crucial public API's, but not for every single method (that just clutters the code).

[lmolkova]

Basically, if we choose to have some rules, especially nitpicking like whitespaces or double empty lines, we can make development better by

• find a way to automate fixing it
• find settings that help everyone not suffer
• if there is no way to avoid it and everyone suffers and it's too hard to develop in this project because of some stylecop rule - we should remove it. Projects like .net core somehow survive without nitpicking or stylecop. So there is a life without it.

Possible solutions could be:

• configure resharper rules and auto-clean up.
• VS feedback to make stylecop work properly.

Particular complaints:

• usings - we chose to have them inside namespace - it is not a default VS behavior and I waste hours fixing it. Together with Resharper auto-import it is awful.

• nitpicking. Whitespace at the end of the line doesn't hurt much.

• VS perf - my VS on my powerful dev boxes (two different) is hanging on this project. It does not happen with other projects. (I blame stylecop).

• prototyping and development process is slow: I have to delete all the whitespaces and other nit things to just run tests

• error list during refactoring is huge. It's hard to find real compilation errors in the list of nitpicks

• stylecop is slow and error list update takes a while

I will be looking into solving some of this problems as time permits. Any feedback is welcome.

[enzian]

@lmolkova That is more to the point and allows for discussion! Let me address some of your bullet points:

find a way to automate fixing it

YES PLEASE! But how? configure resharper rules and auto-clean up isn't really an option here, since many people do not use resharper and/or VS... A bot that cleans things up once PRs are opened is something I've never seen in real life and I don't know if I'd like to have a bot mess in my history...

find settings that help everyone not suffer

Absolutely! And I agree, that Whitespaces at the end of the line doesn't hurt much. So that rule could die for me too, but I fixed all occurrences with one codefix, so that was easy!

error list during refactoring is huge

Yes, I agree! What I would do here in case of a larger refactoring, is to disable WarningsAsErrors and re enable it before opening the PR...

stylecop is slow and error list update takes a while

Not sure stylecop is the bad guy here since builds on the CLI do not suffer from the laggy behavior VS does! So

VS feedback to make stylecop work properly.

would something we should definitely do!

[austinlparker]

OmniSharp seems to have added .editorconfig support (https://github.com/OmniSharp/omnisharp-vscode/releases/tag/v1.21.0) just yesterday, would that do auto clean-up once it makes it into VSC?

[dmccaffery]

There is an .editorconfig extension for vscode that’s been around a long time: https://github.com/editorconfig/editorconfig-vscode

The issues around code fixes for stylecop (or any Roslyn analyzer) has been an on going thing for years now. The analyzers use internal API that Visual Studio calls via reflection to apply fixes. It took a long time, but OmniSharp now supports analyzers (including stylecop); it’s just disabled by default:

https://www.strathweb.com/2019/04/roslyn-analyzers-in-code-fixes-in-omnisharp-and-vs-code/

https://github.com/OmniSharp/omnisharp-roslyn/pull/1076

[SergeyKanzhelev]

most issue easily fixeable from VS itself. At the light bulb next to the violation click the drop down link and say "fix for the entire solution". Somehow I don't see any warnings on the solution now. Was it disabled? Digging now =)

[lmolkova]

So the first step I want to take - usings inside namespaces (as it hurts me the most):

1. There is little-to-no difference in including usings inside or outside of namespace. It boils down to name conflict resolution (which we should not do anyway).

2. Tools (Visual Studio and resharper) add usings automatically outside of namespace declaration by default. There are ways to configure resharper, there are extensions for Visual Studio that can automate it, but it's an extra configuration that has to be applied. And this is unique for this project.

3. .NET Core, ASP.NET Core and majority of other projects put usings outside. The only project I'm aware of (and contribute to) that puts them inside is Microsoft Application Insights (@SergeyKanzhelev I suffer there too!).

4. Personal experience: I waste a lot of my time, fighting with default tooling, moving namespaces, arranging them in order.

5. The only reason we do this as this is default rule in stylecop and internet is full of questions why it is so.

Considering low (or no) value of this rule and a lot of distraction it brings, I suggest we disable it and reformat all code to put using outside.

Thoughts?

[austinlparker]

I concur. I've never worked on a project that had usings inside the namespace, fwiw, and conventionally most .NET projects I've seen have them outside as well.

[simonz130]

I worked on projects that had it in both ways. In projects where usings were inside the namespace it required extra effort to note this in code reviews of new contributors, fix resharper and stylecop, etc. I second Liudmila - it's more efficient from day-to-day perspective to have usings outside the namespace.

[eddynaka]

Hi all, looking at this thread, can i start creating PR's following the StyleCop guidelines in the project? The first i was looking for is about the using inside the class.

What do u think?

[SergeyKanzhelev]

@eddynaka that would be great. I think we agreed on using outside the class. But other stylecop rules might be worth fixing. You can start switching projects from using https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/build/OpenTelemetry.prod.loose.ruleset to https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/build/OpenTelemetry.prod.ruleset One project at a time =).

[eddynaka]

@SergeyKanzhelev , thanks for the tip! that makes thing a lot easier! Just created the first PR. Let me know what do you think :)

[eddynaka]

@SergeyKanzhelev , based on what i did so far and the changes i made, i'm removing all the .rulesets from the projects and maintaining it in the Common.props. With this, i think we dont have any other issue, the ones i found we already solved it.

What else should we do?

[SergeyKanzhelev]

I'd suggest we close this one and will create specific issues for concrete proposals on improving code style