dsccommunity / ActiveDirectoryDsc

This module contains DSC resources for deployment and configuration of Active Directory Domain Services.
MIT License
341 stars 141 forks source link

Code formatting: new line before and after if statement block #489

Closed SSvilen closed 5 years ago

SSvilen commented 5 years ago

Hello,

Is the project standard to have a new line before and after if/ if else statement block? This is not how I write my code and my PRs often have that problem. If it is a standard, than I could do a custom PSScriptAnalyzerRule for that. Always wanted to do something AST related.. :)

johlju commented 5 years ago

I like to have air in the code so it easier for new contributors to read the code, it is easier for the eyes to see blocks, and those blociks are written the same throughout a repo. Most code is written with air, but there are no specific guideline around it (see https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#whitespace). But I love to see this as a new custom rule, and more custom rules for the entire style guideline. Please submit an issue in the repo DscResource.Tests where the custom rules are (in the test framework) https://github.com/PowerShell/DscResource.Tests#psscriptanalyzer-rules. I will add this comment to that new issue you created. Since I not a maintainer of the DscResource.Tests I cannot move this issue there. :/ We need to discuss this with the rest of the community if we should add this too the style guideline. We can that in the issue you create as mentioned above.

So this

$something = $true
if ($something)
{
}
if ()
{
}

I would rather see written as

$something = $true

if ($something)
{
}

if ()
{
}

With the exception of the previous line starts with an open brace. I like to see this as valid.

$something = $true

try
{
    if ($something)
    {
    }
}
johlju commented 5 years ago

While you wait for this being discussed (in the new issue you create) you are more than welcome to create other custom rules so code can easier comply with the style guideline.

Custom rules: https://github.com/PowerShell/DscResource.Tests/blob/dev/DscResource.AnalyzerRules/DscResource.AnalyzerRules.psm1

Custom rules tests: https://github.com/PowerShell/DscResource.Tests/blob/dev/Tests/Unit/DscResource.AnalyzerRules.Tests.ps1

There are two types of tests for each rule, one that calls the custom rule function (to get code coverage), and another test call Invoke-ScriptAnalyzer with the rule to make sure it works there too.

SSvilen commented 5 years ago

Ok, I created the issue in that repo. What kind of custom rule do you think would provide a benefit or what do you, as a maintainer, would like to see?

johlju commented 5 years ago

Suggest starting with making sure keywords like if-statements etc. has lower case characters and is follow by a whitespace, e.g. if ( is correct, any combination of If ( or if( should be caught by a rule. Don't think there are a rule for that yet, and I see it often.

https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#correct-format-for-keywords

You can see if you can build it into the present rule that measures ìf`-statement, or if you could create a rule that measures all keywords as once (if all can be tested equally).

johlju commented 5 years ago

Closing this at this time, let's move discussion to DscResource.Tests.