I think for this project we need to disable SA1200 too. This project almost exclusively has using statements outside the namespace.

As for whether that is a good thing or not... is debatable.

restarted CI due to an update-help connection failure in travis-ci

@SteveL-MSFT @adityapatwardhan @daxian-dbw I've put the list of what is being disabled in the description. Are we all ok with this? I prefer a file only contain a single class, but I agree that it is not what we have been doing.

My opinion on this is that we should do what is best for an Open Source project, whatever that is.

One problem I hear from people acclimating to the code base is that these monolith files are both intimidating and hard to follow. It is often gating low complexity changes from novice contributors.

I personally believe it is easier to track changes from larger numbers of people if those changes are spread over a larger number of files.

To both those ends, one class per file is probably better.

While IntelliSense works in VS Code I haven't difficulties with multi-class files. :-)

Our guideline says that we don't want style changes but CodeFactor fixes is style changes. Should we ignore/remove the requirement or replace with requirement to fix CodeFactor issues?

Also we have a requirement to follow current file style. This makes the code easier to read. If we follow the CodeFactor requirements we break the readability.

I suppose we should first set a top policy:

• should we fix all (approved/not disabled) CodeFactor issues
• should we fix problems for specific rules only

Also we have a requirement to follow current file style.

Except, in practice, reviewers are requesting contributors not do that as often as they enforce it

IMO, for StyleCop or CodeFactor to really help us, we have to first refactor the entire code base to meet the style rules chosen, Then all new changes would just need to follow the rules. I had a recent PR where CodeFactor was complaining about code I didn't even touch just because it was in the same file. I'm not sure how that is even useful.

Fix all 186264 issues is very hude work even if we turn down some rules.

@iSazonov Agreed. This problem is why I ultimately stopped investigating style enforcement, The project has guidelines which are not compatible with style enforcement (use style of surrounding code, don't make style changes), tools for style enforcement are not precise enough to limit detection to just the code added or change in the PR (makes it useless for reviews), and fixing the style globally so that rules and tools can be useful is a massive undertaking. Also, trying to fix the problems in a single area (like web cmdlets) is a strain on the reviewers and the contributor, there are not enough contributors and reviewers to tackle the problem in just one area, let alone globally.

Some issues can be easily fixed:
"" -> string.Empty
String -> string

It is up to 1-2% issues.

Do you know of other problems that can be easily fixed?

• Char -> char
• "Closing Curly braces must be followed by a blank line"
• "Elements must be separated by a blank line"
• Single line comments must begin with a space

Basically, the white space rules are all easy wins and there are a ton of them.

We could also properly configure "The file header must contain a copyright tag" so that it doesn't false-alarm or just disable SA1634.

SA1518 should be set to required.

These are some of the easy low risk wins, I think:

• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Maintainability&groupId=259
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Maintainability&groupId=281
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Maintainability&groupId=46
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Maintainability&groupId=22
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=81
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=72
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=264
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=93
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=80
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=83
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=95
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=6
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=40
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=85
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=39
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=38
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=18
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=8
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=82
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=9
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=269
• https://www.codefactor.io/repository/github/powershell/powershell/pull/6930?category=Style&groupId=5

@markekraus Thanks!

SA1518 is on by default.

the white space rules are all easy wins and there are a ton of them.

Maybe formatting tool from .NET code formatter tool can help? Our docs says :-)

We also run the .NET code formatter tool regularly to keep consistent formatting.

Where can get a config for the CodeFormatter?

The default settings were used when I ran the code formatter.

Some folks may like StyleCop, but I think it will discourage some contributors - it's far too picky.

And I've stated this before, but I strongly oppose one class per file - that decision should be up to the author and primary maintainer of the feature.

And to provide some data behind my objection to 1 class per file - there are > 3200 types in System.Management.Automation and ~740 cs files, so the average is >4 types per file. Going to 1 type per file means quadrupling the number of files in the project.

I'm not opposed to targeted improvements, but please don't blindly apply an arbitrary policy.

@lzybkr Thanks! I agree that we should disable rule for "one class per file".
What do you think about rule which says about prefixes like "this."? CodeFactor reports tons such issues.

I'd also ignore the this. prefix warning. More unnecessary churn if you change it, and much riskier to use a regex to "fix".

I'm fine with the proposed disabled settings. In general, we should error on the side of productivity and lowering the barrier for contributions.

@lzybkr

much riskier to use a regex to "fix".

I did not use a script with Regex. I did the changes in VS Code step by step selecting a pattern and then visually checking the results to exclude false changes, then commit changes for the pattern, then select next pattern.

Disable SA1101 and SA1126 - up to 25% win.

I think we should disable

• SA1600 ElementsMustBeDocumented rule because it give strange issues like

  PowerShell/src/System.Management.Automation/namespaces/FileSystemProvider.cs

  Line 7344 in 4c925f5

  public int Type;

• SA1201 ElementsMustAppearInTheCorrectOrder and SA1202 ElementsMustBeOrderedByAccess - I believe we'll never reorder all our files.

@iSazonov - removing this. can change a program's behavior. Consider:

    class C {
        int value;
        void f(int v1, int value) {
            this.value = v1;
        }
    }

If you remove this. - you will be assigning to the parameter. I'm not aware of tools that will help detect this change - it's very manual. This is why I said it's risky.

@lzybkr I means disabling rules SA1101 and SA1126 not changing real code. I believe you agree with the disabling.

In #6949 @lzybkr suggest disable SA1131 UseReadableConditions

I think we can merge, look how CodeFactor statistics will be changed for master branch and continue adjust the settings in follow PRs.