Style: Update StyleCop rules - #8500

I wonder that we should think only about 'Visit*' methods that take up <1%.
I'd do not turn out the rule and related rules below.

This rule complains when you have fields defined without blank lines between them. In my PR reviews, I almost never see other type members that has no blank lines between them (usually have more than 1 blank lines). Given that, I don't think this rule is very useful.

Isn't the point of a style rule to assist in keeping with consistency of existing style? It's great that we do apply this pretty widely anyway, but having the rule call it out makes it easier for reviewers to spot, and lessens the likelihood it would be overlooked. Minimizing human error and overlooked style issues in PRs is generally a good thing, I think.

When it comes to whether or not to disable a style rule, I consider the following 3 aspects:

1. How bad are the occurrences of violation in the code base, and how expensive to fix them.
   • If many occurrences of violation in the existing code base are pretty bad and should be fixed, then we should fix them gradually no matter how expensive to do so.
   • If most occurrences of violation doesn't really matter that much (not really an issue of readability or maintenance), then
     • if it's not expensive to fix them, then fix them
     • if it's expensive to fix them, especially if some of the occurrences are legit or on purpose, then continue on (2) and (3) below to see if the rule can be disabled.
2. How often does the problem indicated by the rule appear in new changes (PR reviews).
   • This for now is completely based on my personal PR review experience.
     • Take this rule as an example, I personally never see type elements adjacent to each other except for fields and properties in PR reviews.
   • This gives me a rough clue about how useful this rules is.
3. Is this rule ambiguous -- are there significant amount of places that intentionally use a style other than what the rule suggests.
   • When a rule is ambiguous, it will be a problem to enforce it. Not every PR author will know about the difference and reason behind it, even if you have it documented somewhere.

If the current violations meet the following conditions, then I consider to disable the rule:

• don't really matter that much and it's expensive to fix them, AND,
• the violation of the rule seldom happen based on the past PR review experiences, AND,
• the rule is ambiguous,

We cannot enforce the CodeFactor checks as our CI gate unless we have a clean code base, so we do need to take (1) in consideration.
Before we enforce it as the CI gate, I doubt that reviewers or authors will seriously look at the CodeFactor issues. Even if they look at the issues, it's unclear to them whether they have to fix all violations because of the messy state of the code base itself.

Steve Lee (@SteveL-MSFT) Aditya Patwardhan (@adityapatwardhan) Travis Plunk (@TravisEz13) Can you please also weigh in on this discussion?

By the way, I totally agree to only leave those intentional violations and fix the rest in our code base in general, because people copy code styles from the existing code in the files they are touching.
However, we should still consider disabling the rule as long as it's ambiguous and violations seldom happen in new changes.

Agree that the goal here is to have a set of rules so that CodeFactor (or Codacy) can help enforce style rules rather than having style discussions being spent on code reviews. The only way to do that is to get to a passing grade.

This one has >4000 violations in the current code base so I tend to agree that it's not worth the time right now to fix and review the fix (which would definitely take longer than the fix itself) for the benefit.

Before we enforce it as the CI gate, I doubt that reviewers or authors will seriously look at the CodeFactor issues. Even if they look at the issues, it's unclear to them whether they have to fix all violations because of the messy state of the code base itself.

I think we might not force it at all. Over the past six months, we have fixed a huge number of style problems without the forcing. Almost every PR either has no style problem or fixes some amount.

• Even without forcing, the presence of CodeFactor helps us make the code cleaner.
• Even if we can't fix all the style problems, an enabled rule motivates us to create clean code.
• I see that CodeFactor reduced to zero our style requests and discussions in PRs.

I think we use only one very rare related pattern like if ( … ) { … }.
So I'd not turn out the rule.

There are 712 violations of this rule, and yes, most are that if pattern. Some catch { .. } too.
I don't think it's worthwhile to fix them.

As I said above we shouldn't force CodeFactor. In the case we don't need to fix all the style issues and enabled rules will motivate us to push clean code.