Style: Update StyleCop rules#8500
Conversation
Settings.StyleCop
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
When it comes to whether or not to disable a style rule, I consider the following 3 aspects:
- 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.
- 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.
- This for now is completely based on my personal PR review experience.
- 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.
There was a problem hiding this comment.
@SteveL-MSFT @adityapatwardhan @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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Settings.StyleCop
Outdated
There was a problem hiding this comment.
I think we use only one very rare related pattern like if ( … ) { … }.
So I'd not turn out the rule.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
|
@SteveL-MSFT @daxian-dbw Today we have only two rule under question (and 3 we need re-enable). Can we get conclusion and merge? In last month we fixed huge amount of style issues and now we have almost no PR with CodeFactor style issues. So I don't understand why we need move to Codacy? |
|
@iSazonov, @daxian-dbw is on vacation for one more week, I think we can wait until he gets back. Don't worry if this PR is marked stale, the bot won't delete the branch. |
|
@SteveL-MSFT Thanks! There is two open questions which we need resolve. We could continue with you, @adityapatwardhan and @TravisEz13. |
|
@daxian-dbw Could we merge with the rules for which we have a consensus and see the results? |
|
@iSazonov Yes, will get back to this PR next Monday! |
|
@daxian-dbw Friendly ping. |
|
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
|
Thanks stale bot ... |
|
@PoshChan Please remind me in 42 hours |
|
@daxian-dbw, will remind you in 42 hours |
|
@daxian-dbw, this is the reminder you requested 42 hours ago |
|
@iSazonov I reverted all controversial changes in the PR. |
|
@daxian-dbw Thanks! I will merge "as is" we will see what we can do better. |
PR Summary
Update StyleCop rules, including the individual rules and project-scope rules.
With the updated rules, 7965 CodeFactor issues are fixed/ignored.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests