Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Add Style Guideline Example for How Braces in Switch Block should be Used #483

Open
PlagueHO opened this issue Feb 2, 2019 · 10 comments
Open
Labels
discussion The issue is a discussion.

Comments

@PlagueHO
Copy link
Contributor

PlagueHO commented Feb 2, 2019

There is currently no example of how braces in a switch block should look.

Options:
1.

switch ($condition)
{
   'a' { Statement }
   'b' { Statement }
}
switch ($condition)
{
   'a'
   {
      Statement
    }
   'b'
    {
        Statement
    }
}
switch( $condition )
{
    { $_ -eq 'a' -or $_ -eq 'b' }
        {
            Statement1
        }
    { $_ -gt 0 -and $_ -lt 10 }
        {
            Statement2
        }
    { $_ -match "regex" }
        {
            Statement3
        }
}

@mhendric, @johlju - your thoughts? Should we add this and what is the preferred method. I've used 2, but only because I assumed that was covered - which it isn't. I'm good with either way.

I think 2 is easier to enforce because 1 will result in two styles potentially:
1.

switch ($condition)
{
   'a' { Statement }
   'b' { Statement }
}

and
1.

switch ($condition)
{
   'a' {
      Statement 1
      Statement 2
    }
   'b' {
      Statement 1
      Statement 2
     }
}

What is everyone's thoughts?

@essentialexch
Copy link

You left the style necessary for complex cases. For example

switch( $condition )
{
    { $_ -eq 'a' -or $_ -eq 'b' }
        {
            Statement1
        }
    { $_ -gt 0 -and $_ -lt 10 }
        {
            Statement2
        }
    { $_ -match "regex" }
        {
            Statement3
        }
}

And here you see my preferred style too. :-)

@PlagueHO
Copy link
Contributor Author

PlagueHO commented Feb 2, 2019

Thanks @swngdnz - added to the list 😁

@stale
Copy link

stale bot commented Mar 4, 2019

This issue has been automatically marked as stale because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is labelled with any of the work labels (e.g bug, enhancement, documentation, or tests) then the issue will not auto-close.

@stale stale bot added the stale The issue or pull request was marked as stale because there hasn't been activity from the community. label Mar 4, 2019
@PlagueHO
Copy link
Contributor Author

PlagueHO commented Mar 5, 2019

Bump

@stale stale bot removed the stale The issue or pull request was marked as stale because there hasn't been activity from the community. label Mar 5, 2019
@gaelcolas gaelcolas added the discussion The issue is a discussion. label Mar 12, 2019
@gaelcolas
Copy link
Contributor

I'm not a fan with enforcing this, as different usage of the switch statement might call for different layout.
In some (rare) cases it's much more concise and readable to to have a single line for the case statement:

$value = switch($blah) {
    'a'        { 'A' }
    1          { 'A' }
    default { 'B' }
}

My point is that we should recommend some format, but probably let the maintainers decide if they want to enforce a way or another in their repository.

@gaelcolas
Copy link
Contributor

Just came across an Illustration of the above in a PR:
image

I personally prefer the former layout... It does against vscode auto-formatting feature though, so I'm not that bothered, I can adapt.

@johlju
Copy link
Contributor

johlju commented Mar 17, 2019

We have the style guideline that says open brace should be in a separate row, so then the code gets stretched out in scenarios like these.
I think having the style guideline as-is is good because we have one way of writing the code. Of course there are a lot of different ways to write code, but the if we should improve review time by adding PSSA rules for style, then I don’t think we can relax the rules in one area but not in another. Well we can if we the community choose of course.
And using VS Code auto-formatting is a time saver too, and then if a contributer was write the open and closing braces differently the auto-formatting would add them back and making more changes than the PR intended.

I’m always open to style changes if it improves that everyone writes the code the same and supports “easily written” future PSSA rules and auto-formatting.

@PlagueHO
Copy link
Contributor Author

I do prefer the more concise layout at @gaelcolas mentioned. But only if there is a single line of code in each block. So perhaps the rule could be "Compact" when only a single line in each block is used. E.g.

$value = switch($blah)
{
    'a'        { 'A' }
    1          { 'A' }
    default { 'B' }
}

And expanded otherwise:

$value = switch($blah) {
    'a'       
    {
                Write-Verbose -Message 'Line 1'
                Write-Verbose -Message 'Line 2'
    }
    1
    {
                Write-Verbose -Message 'Line 1'
                Write-Verbose -Message 'Line 2'
    }
    default
    {
                Write-Verbose -Message 'Line 1'
                Write-Verbose -Message 'Line 2'
    }
}

That said, sometimes we use switch blocks when we should possibly have defined an enumeration instead. So that kind of block may be an indicator that some refactoring could be made.

I'd suggest thought that this isn't allowed:

$value = switch($blah) {
    'a'        { 'A' }
    1          { 'A' }
    default { 'B' }
}

@gaelcolas
Copy link
Contributor

My example was only to illustrate that enforcing is not necessarily better than recommending (though, agreed, it's a 1-line content edge case).
But I agree with what @johlju pointed out, it might be easier to just enforce what's configurable in vscode, and preferably the defaults to avoid formatting hell in PR reviews.

As for having one and unique standard across all repositories (for cosmetic changes that don't affect quality), I'd argue that it mainly serves you both (not many people are doing as much work across so many repositories).
I'm not saying it shouldn't be the case and I do want to make it easier for your eyes!

I also agree that it helps people contributing to multiple repositories, and it should be enforced by the maintainers in any given repository.

But it repulses some would-be contributors/maintainers (I had this direct feedback), not in existing repository because people happily comply with established standards, but for new resources they want to share and include in the Resource Kit.

In conclusion, I'd say that unless we can configure specific auto-formatting in vscode (aligned with options 1,2,3) and someone to ask for it specifically, I'm happy to have the guidelines updated and enforced with option 2 and 3 only as suggested by @PlagueHO.

Should we bring this update to next Community call?

@johlju
Copy link
Contributor

johlju commented Mar 26, 2019

I’m not sure we can ask the repositories outside of DSC Resource Kit that they must enforce the formatting guidelines. I know we say they should follow the guidelines, but that should be just that, a guideline. Maybe they can just run there own guideline which overrides the “central” one.

But repos owned by the PowerShell GitHub account I think should enforce the guideline for all PRs.

@SteveL-MSFT SteveL-MSFT added this to Discussion in powershell/dscresources May 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion The issue is a discussion.
Projects
Development

No branches or pull requests

4 participants