Skip to content

Style: Update StyleCop rules#8500

Merged
iSazonov merged 3 commits intoPowerShell:masterfrom
daxian-dbw:style-2
Apr 3, 2019
Merged

Style: Update StyleCop rules#8500
iSazonov merged 3 commits intoPowerShell:masterfrom
daxian-dbw:style-2

Conversation

@daxian-dbw
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw commented Dec 21, 2018

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

@daxian-dbw daxian-dbw requested a review from iSazonov December 21, 2018 01:16
@daxian-dbw daxian-dbw added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Dec 21, 2018
@daxian-dbw daxian-dbw changed the title Update StyleCop rules Style: Update StyleCop rules Dec 21, 2018
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@daxian-dbw daxian-dbw Dec 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@daxian-dbw daxian-dbw Dec 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member

@SteveL-MSFT SteveL-MSFT Dec 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@stale
Copy link
Copy Markdown

stale bot commented Jan 22, 2019

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.
Thank you for your contributions.
Community members are welcome to grab these works.

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Feb 7, 2019

@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 iSazonov self-assigned this Feb 8, 2019
@SteveL-MSFT
Copy link
Copy Markdown
Member

@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.

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Feb 9, 2019

@SteveL-MSFT Thanks! There is two open questions which we need resolve. We could continue with you, @adityapatwardhan and @TravisEz13.

@iSazonov
Copy link
Copy Markdown
Collaborator

@daxian-dbw Could we merge with the rules for which we have a consensus and see the results?

@daxian-dbw
Copy link
Copy Markdown
Member Author

@iSazonov Yes, will get back to this PR next Monday!

@iSazonov
Copy link
Copy Markdown
Collaborator

@daxian-dbw Friendly ping.

@stale
Copy link
Copy Markdown

stale bot commented Mar 30, 2019

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.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Mar 30, 2019
@daxian-dbw
Copy link
Copy Markdown
Member Author

daxian-dbw commented Mar 30, 2019

Thanks stale bot ...
This PR has been constantly pushed back by other higher priority work items ... not good 😞
This time I promise to get this PR done by end of next week.

@stale stale bot removed the Stale label Mar 30, 2019
@daxian-dbw
Copy link
Copy Markdown
Member Author

@PoshChan Please remind me in 42 hours

@PoshChan
Copy link
Copy Markdown
Collaborator

@daxian-dbw, will remind you in 42 hours

@PoshChan
Copy link
Copy Markdown
Collaborator

PoshChan commented Apr 1, 2019

@daxian-dbw, this is the reminder you requested 42 hours ago

@daxian-dbw
Copy link
Copy Markdown
Member Author

@iSazonov I reverted all controversial changes in the PR.

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Apr 3, 2019

@daxian-dbw Thanks! I will merge "as is" we will see what we can do better.

@iSazonov iSazonov added this to the 6.3.0-preview.1 milestone Apr 3, 2019
@iSazonov iSazonov merged commit 74b71f2 into PowerShell:master Apr 3, 2019
@daxian-dbw daxian-dbw deleted the style-2 branch April 3, 2019 20:11
@iSazonov iSazonov mentioned this pull request Apr 5, 2019
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants