Powershell: Need tooling to enforce style guide

[SteveL-MSFT]

Everyone agrees that from a style perspective, the intent is to have consistency in the code to make it more readable and maintainable. Update https://github.com/PowerShell/PowerShell/blob/master/docs/dev-process/coding-guidelines.md as needed, but should be enforced by tooling and not by PR comments.

• [ ] enforcement on the CI side
• [x] enforcement on the developer side (part of Start-PSPester?)

[iSazonov]

We already have PR #3504 for CI side and request from @lzybkr for developer side (git commit hooks) in the PR.

[SteveL-MSFT]

@iSazonov you can close this once that PR is merged. Thanks!

[iSazonov]

The PR s problematic - we can't merge it until we fix all files in the repo.
Also I believe we should address git commit hooks in another PR - I never write such hooks.

/cc @lzybkr

[daxian-dbw]

@TravisEz13 Could you please comment? I think you have more experience on this and have some ideas about how we can gradually roll out the files that are covered by the formatting checks.

[iSazonov]

I see GitHub introduce Checks. Can we use this?

[TravisEz13]

I've seen other teams using https://www.codefactor.io/

[iSazonov]

Screenshots look ver interesting. Can we try this in the repo?

[TravisEz13]

@SteveL-MSFT @daxian-dbw @adityapatwardhan What do you think? Can we try codefactor.io?

[SteveL-MSFT]

@TravisEz13 I'm open to trying it out. I'll set it up.

[adityapatwardhan]

I am open to trying it as long as it does not block PRs from being merged, at least to begin with.

[SteveL-MSFT]

https://www.codefactor.io/repository/github/powershell/powershell/overview/master

[iSazonov]

Thanks! Looks great.

Have your own StyleCop rules? No problem, simply add Settings.StyleCop file to the root of this branch.

We can configure rules which we want.

[iSazonov]

Overall grade is C - Satisfactory.
We could show the status on our main page.
Perhaps we should work now to get into B status.

I selectively reviewed the results of CodeFactor.
Most commonly found Duplicate Code and Complex Method. Others are rare.
Seems we can easily address most of CodeFactor Issues.

[iSazonov]

Duplication rules should be disabled. - it seems completely useless.

/cc @markekraus

[iSazonov]

I believe we should disable unneeded rules and _fix_ most of style issues.
Motivation - conservative policy is very good for a closed project, for an open project we have to continuously discuss the style issues that slows code reviews and leads to the perplexity of contributors.

[markekraus]

@iSazonov I wouldn't call that rule useless. It shows code reuse where we could refactor to a utility method or something. But refactoring those issues may be high risk.

[iSazonov]

Then we have to CodeFactor and live like before.

[markekraus]

@iSazonov I wasn't saying we shouldn't disable the rule, just that it wasn't completely useless. :)

[iSazonov]

We need new policy and roadmap.

[TravisEz13]

I agree it isn't useless, but many of the suggested refactoring to remove duplication is risky. It would be nice to be able to leave it on as informational if that is an option.

[iSazonov]

duplication

I see only 519 issues so it's not intrusive and we can leave this as is.

[powercode]

@SteveL-MSFT
I think it is a horrible experience with a style checker running on PRs but without a tool that automatically formats the code in the expected way.

Takes away quite a lot of the joy of fixing things.

[SteveL-MSFT]

@powercode We're open to trying other tools if you're aware of any.

[iSazonov]

CodeFactor is only control tool. We could use https://github.com/dotnet/codeformatter but it also requires a lot of effort. Anf it is only formatting tool and it doesn't fix all style issues.

[powercode]

I just think we should do this the other way around. Start with giving people the tools to format the source, and then start to enforce it on pull requests.

This is of course a matter of opinion, but I don't think the gain (if any) in readability and maintainability is worth the hassle.

I'm working on getting a ReSharper profile setup for the project that has formatting options that are at least close to Code Factor. I know it is a commercial tool, but a lot of C# devs has it. That could east the pain a bit.

I installed StyleCop locally, but got lots of errors with spelling, and didn't figure out how to make it use the existing spelling dictionary.

In summary, it is not so much the tool I object to, as the backward order this is introduced in.
Tools first, then checks, not the other way around.

[iSazonov]

CodeFormatter doesn't work with PowerShell Core csproj files https://github.com/dotnet/codeformatter/issues/256

[iSazonov]

From @daxian-dbw

About the style rules, @TravisEz13, @adityapatwardhan and I went through all issues reported from CodeFactor a few weeks back, and we got a list of rules that we think should be turned off. Here is the list:
1305: SA1305FieldNamesMustNotUseHungarianNotation
1502: SA1502ElementMustNotBeOnSingleLine
1310: SA1310FieldNamesMustNotContainUnderscore,
1204: SA1204StaticElementsMustAppearBeforeInstanceElements
1009: SA1009ClosingParenthesisMustBeSpacedCorrectly
1501:SA1501StatementMustNotBeOnSingleLine
1513:SA1513ClosingBraceMustBeFollowedByBlankLine
1306:SA1306FieldNamesMustBeginWithLowerCaseLetter
1308:SA1308VariableNamesMustNotBePrefixed
1013:SA1013ClosingBracesMustBeSpacedCorrectly
1500:SA1500BracesForMultiLineStatementsMustNotShareLine
1010:SA1010OpeningSquareBracketsMustBeSpacedCorrectly
1026:SA1026CodeMustNotContainSpaceAfterNewKeywordInImplicitlyTypedArrayAllocation
1008:SA1008OpeningParenthesisMustBeSpacedCorrectly
1311:SA1311StaticReadonlyFieldsMustBeginWithUpperCaseLetter
1025:SA1025CodeMustNotContainMultipleWhitespaceInARow
1012:SA1012OpeningBracesMustBeSpacedCorrectly
1215:SA1215InstanceReadonlyElementsMustAppearBeforeInstanceNonReadonlyElements
1214:SA1214ReadonlyElementsMustAppearBeforeNonReadonlyElements
1210:SA1210UsingDirectivesMustBeOrderedAlphabeticallyByNamespace
1609:SA1609PropertyDocumentationMustHaveValue

The rules about "one parameter per line" for method definition or invocation is not in this list, but in my opinion, we probably should disable that rule because 1) it doesn't bring much benefit -- as long as the parameters are well aligned, there is no readability issue; 2) there are too many instances in our existing code base that are violating that rule, and fixing all doesn't worth the time.

The following document rules make sense for public members, but they are applied to non-public members too, which causes a lot of noise data.

1611:SA1611ElementParametersMustBeDocumented
1615:SA1615ElementReturnValueMustBeDocumented
1606:SA1606ElementDocumentationMustHaveSummaryText

[iSazonov]

Current formatting options from Roslyn (from package version 2.8.2)
(Need Microsoft.CodeAnalysis.CSharp.Workspaces.dll and Microsoft.CodeAnalysis.Workspaces.dll)

All the rules we need are enabled/disabled?

PS > Add-Type -AssemblyName .\Microsoft.CodeAnalysis.CSharp.Workspaces.dll
PS > [Microsoft.CodeAnalysis.CSharp.Formatting.CSharpFormattingOptions] | Get-Member -Static -MemberType Properties | Select -ExpandProperty Name | % { "$_ = $([Microsoft.CodeAnalysis.CSharp.Formatting.CSharpFormattingOptions]::$_.DefaultValue)" }

IndentBlock = True
IndentBraces = False
IndentSwitchCaseSection = True
IndentSwitchCaseSectionWhenBlock = True
IndentSwitchSection = True
LabelPositioning = OneLess
NewLineForCatch = True
NewLineForClausesInQuery = True
NewLineForElse = True
NewLineForFinally = True
NewLineForMembersInAnonymousTypes = True
NewLineForMembersInObjectInit = True
NewLinesForBracesInAccessors = True
NewLinesForBracesInAnonymousMethods = True
NewLinesForBracesInAnonymousTypes = True
NewLinesForBracesInControlBlocks = True
NewLinesForBracesInLambdaExpressionBody = True
NewLinesForBracesInMethods = True
NewLinesForBracesInObjectCollectionArrayInitializers = True
NewLinesForBracesInProperties = True
NewLinesForBracesInTypes = True
SpaceAfterCast = False
SpaceAfterColonInBaseTypeDeclaration = True
SpaceAfterComma = True
SpaceAfterControlFlowStatementKeyword = True
SpaceAfterDot = False
SpaceAfterMethodCallName = False
SpaceAfterSemicolonsInForStatement = True
SpaceBeforeColonInBaseTypeDeclaration = True
SpaceBeforeComma = False
SpaceBeforeDot = False
SpaceBeforeOpenSquareBracket = False
SpaceBeforeSemicolonsInForStatement = False
SpaceBetweenEmptyMethodCallParentheses = False
SpaceBetweenEmptyMethodDeclarationParentheses = False
SpaceBetweenEmptySquareBrackets = False
SpacesIgnoreAroundVariableDeclaration = False
SpaceWithinCastParentheses = False
SpaceWithinExpressionParentheses = False
SpaceWithinMethodCallParentheses = False
SpaceWithinMethodDeclarationParenthesis = False
SpaceWithinOtherParentheses = False
SpaceWithinSquareBrackets = False
SpacingAfterMethodDeclarationName = False
SpacingAroundBinaryOperator = Single
WrappingKeepStatementsOnSingleLine = True
WrappingPreserveSingleLine = True

[iSazonov]

Sample of formatting with codeformatter that I compiled ##7263.
If it is Ok I'll continue to format remaining projects.

[joeyaiello]

@SteveL-MSFT is this okay to close now?

[vexx32]

Do we have an omnisharp.json in the repo with the mentioned style rules set for VS Code users? Might be a good idea to do so if we haven't.

Also, I opened another issue already, but codefactor should definitely come back for those edge cases and misc recommendations.

[SteveL-MSFT]

@joeyaiello no, we need to integrate with CI

[iSazonov]

@vexx32 We have .editorconfig as unified config for all editors.