Add CheckInnerBrace and CheckPipe options to PSUseConsistentWhitespace #1092

bergmeister · 2018-11-03T13:40:01Z

PR Summary

Closes #1065 and #1113 by adding 2 new rule options (CheckInnerBrace and CheckPipe) to PSUseConsistentWhitespace that check if there is a space after the opening brace and before the closing brace (CheckInnerBrace) and that a pipe is surrounded by spaces. Those options are turned on by default. Settings files are being adapted. Please see the Pester tests for special cases such as newlines or line continuation characters where we try to be as acceptable as possible to different code styles and don't emit a warning.
Question: should those options maybe be integers instead of booleans to specify the number of space characters (since someone could also want 0, 2 or 3)?

This also means that we should add 2 new powershell.codeFormatting settings to the vs code extension that control those 2 new options. cc @TylerLeonhardt @rjmholt

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking -> Since those checks are new, the new violations might break people but given that it makes sense to enable them by default and people can disable the new options, this small breaking change makes sense to me.
Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe

…DO: cater for newlines

TylerLeonhardt · 2018-11-06T06:24:37Z

@bergmeister once this gets merged in, can you open an issue on vscode-powershell to add the settings?

JamesWTruher · 2018-12-06T23:03:29Z

Tests/Rules/UseConsistentWhitespace.tests.ps1

+            $ruleConfiguration.CheckSeparator = $false
+        }
+
+        It "Should find a violation if there is no space after pipe" {


Could these 2 be simplified into Get-Item|foo and look for 2 errors?

The existing test helper method Test-CorrectionExtentFromContent does not support more than 1 violation. We could adapt it but I generally prefer having more separated tests over tests that test 2 things tbh.

JamesWTruher · 2018-12-06T23:04:58Z

Tests/Rules/UseConsistentWhitespace.tests.ps1

+        }
+
+        It "Should not find a violation if there is 1 space before and after a pipe" {
+            $def = 'Get-Item | foo'


should we be checking for things like Get-Item | foo? (there are more than one space in there)

Yes, I've added 2 new test cases for that now

JamesWTruher · 2018-12-06T23:06:39Z

Tests/Rules/UseConsistentWhitespace.tests.ps1

+
+        It "Should not find a violation if there is 1 space inside empty curly braces" {
+            $def = @'
+if($true) { }


should there be a violation if there is more than 1 space?

There will be a violation and it will correct it to have only exactly 1 whitespace between braces. I added a test case for that as well

JamesWTruher · 2018-12-06T23:14:10Z

Rules/UseConsistentWhitespace.cs

+            {
+                if (lCurly.Next == null
+                    || !IsPreviousTokenOnSameLine(lCurly)
+                    || lCurly.Next.Value.Kind == TokenKind.LCurly


does this mean the rule will skip things like {{{{{?

yes, you are right, I've taken that line out now so that it formats now to { { { foo } } }, etc. if there is more than 1 consecutive brace

JamesWTruher · 2018-12-06T23:18:55Z

Rules/UseConsistentWhitespace.cs

+                }
+            }
+
+            foreach (var pipe in tokenOperations.GetTokenNodes(TokenKind.Pipe))


can these loops be combined in some way?

Are you worried about it in terms of performance or just code maintenance? In the latter case, it's tricky, in the former case we call tokenOperations.GetTokenNodes foreach rule setting separately, so 6 times more than needed. I am not sure if querying for all token kinds once and then putting them into a separate list would really be faster but I am not an expert at performance optimisation.

rjmholt · 2018-12-07T03:42:22Z

My only concern is with the proposed settings. If these are analysis rules rather than formatting rules, then I think there's a better investment to be made in PowerShell/vscode-powershell#1443.

If we add the proposed configurations above, and then implement PowerShell/vscode-powershell#1443, which will take precedence?

I know we've procrastinated on that issue, but given that it's had some time for feedback, we should just implement that, and include some pre-configured setting sets with a sane default.

bergmeister · 2018-12-15T14:02:54Z

@rjmholt They are options of a formatting rule, not analysis rule (although they look the same from a code perspective), therefore their main usage will be from Invoke-Formatter not from Invoke-ScriptAnalyzer (although can also do the latter technically), hence why it would make sense to add vs-code settings similar to the other rule configurations of this rule in powershell.codeformatting that are just wrappers around one PSSA formatting rule.
Your vscode-powershell issue is about having an alternative to a PSSA settings file for analysis, this rule is more interesting to be used for formatting, not for analysis

rjmholt · 2018-12-17T19:57:22Z

Ok good, just making sure these didn't meet the proviso of my comments. In that case I'm happy with everything

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe

…e the closing brace would not be correctly replaced

…d test case for that

…er method to inform about limited functionality

…ests

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe # Conflicts: # Rules/Strings.resx

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe

# Conflicts: # Tests/Engine/InvokeScriptAnalyzer.tests.ps1

PowerShell#1092) * Check for 1 whitespace AFTER curly brace in new InnerCurly option * check for one space in closing of inner brace * add check for pipes. TODO: messages * update settings files and add first set of tests for curly braces. TODO: cater for newlines * fix new line handling for innerbrace * fix suggested corrections and add another test case for inner brace * order settings alphabetically and add tests for checkpipe * fix test that returned 2 warnings now due to checkinnerbrace * fix innerPipe and write documentation * tweak backtick scenarios * fix 1 failing test * customise warning messages * swap messages to be correct * add more test cases and fix small bug whereby the missing space before the closing brace would not be correctly replaced * enforce whitespace also if there is more than 1 curly brace. TODO: add test case for that * add test cases for nested parenthesis and add validation to test helper method to inform about limited functionality * add test case for more than 1 space inside curly braces and tidy up tests

bergmeister added 12 commits September 25, 2018 20:25

Check for 1 whitespace AFTER curly brace in new InnerCurly option

7e740b2

check for one space in closing of inner brace

cebbaad

add check for pipes. TODO: messages

5f444bd

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

1052423

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe

Merge branch 'development' into UseConsistenWhiteSpace_InnerCurlyAndPipe

1fbb541

update settings files and add first set of tests for curly braces. TO…

60885b0

…DO: cater for newlines

fix new line handling for innerbrace

692969e

fix suggested corrections and add another test case for inner brace

5ab67c1

order settings alphabetically and add tests for checkpipe

a4f5c8d

fix test that returned 2 warnings now due to checkinnerbrace

b45cce5

fix innerPipe and write documentation

0668df9

tweak backtick scenarios

d74412e

bergmeister added Issue - Enhancement Area - Rules Area - Formatter Feature Request labels Nov 3, 2018

bergmeister added this to the 1.18 milestone Nov 3, 2018

bergmeister self-assigned this Nov 3, 2018

bergmeister requested a review from JamesWTruher November 3, 2018 13:40

bergmeister added 3 commits November 3, 2018 13:43

fix 1 failing test

43b5882

customise warning messages

73edc10

swap messages to be correct

6b362d9

JamesWTruher reviewed Dec 6, 2018

View reviewed changes

bergmeister mentioned this pull request Dec 19, 2018

Code Formatting : Whitespacing one line hashtable #1113

Open

bergmeister added 2 commits December 19, 2018 13:53

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

c872d1c

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe

add more test cases and fix small bug whereby the missing space befor…

ff3e3d1

…e the closing brace would not be correctly replaced

bergmeister added 3 commits December 19, 2018 14:49

enforce whitespace also if there is more than 1 curly brace. TODO: ad…

e4d6879

…d test case for that

add test cases for nested parenthesis and add validation to test help…

fb57292

…er method to inform about limited functionality

add test case for more than 1 space inside curly braces and tidy up t…

196b112

…ests

bergmeister added 3 commits January 9, 2019 21:23

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

0bc8a97

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

d15ba9d

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe # Conflicts: # Rules/Strings.resx

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

c46ae83

…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe

bergmeister added the (Re-)Review Needed Feedback has been addressed during PR stage or is required in the first place. label Jan 29, 2019

Merge branch 'development' into UseConsistenWhiteSpace_InnerCurlyAndPipe

59963b2

# Conflicts: # Tests/Engine/InvokeScriptAnalyzer.tests.ps1

rjmholt approved these changes Mar 5, 2019

View reviewed changes

bergmeister merged commit 484f405 into PowerShell:development Mar 5, 2019

bergmeister mentioned this pull request Jan 23, 2021

Formatter inserts redundant whitespace in ValidateSet attribute #1468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CheckInnerBrace and CheckPipe options to PSUseConsistentWhitespace #1092

Add CheckInnerBrace and CheckPipe options to PSUseConsistentWhitespace #1092

bergmeister commented Nov 3, 2018 •

edited

Loading

TylerLeonhardt commented Nov 6, 2018

JamesWTruher Dec 6, 2018

bergmeister Dec 19, 2018 •

edited

Loading

JamesWTruher Dec 6, 2018 •

edited

Loading

bergmeister Dec 19, 2018 •

edited

Loading

JamesWTruher Dec 6, 2018

bergmeister Dec 19, 2018

JamesWTruher Dec 6, 2018

bergmeister Dec 19, 2018 •

edited

Loading

JamesWTruher Dec 6, 2018

bergmeister Dec 19, 2018 •

edited

Loading

rjmholt commented Dec 7, 2018

bergmeister commented Dec 15, 2018 •

edited

Loading

rjmholt commented Dec 17, 2018

Add CheckInnerBrace and CheckPipe options to PSUseConsistentWhitespace #1092

Add CheckInnerBrace and CheckPipe options to PSUseConsistentWhitespace #1092

Conversation

bergmeister commented Nov 3, 2018 • edited Loading

PR Summary

PR Checklist

TylerLeonhardt commented Nov 6, 2018

JamesWTruher Dec 6, 2018

Choose a reason for hiding this comment

bergmeister Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

JamesWTruher Dec 6, 2018 • edited Loading

Choose a reason for hiding this comment

bergmeister Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

JamesWTruher Dec 6, 2018

Choose a reason for hiding this comment

bergmeister Dec 19, 2018

Choose a reason for hiding this comment

JamesWTruher Dec 6, 2018

Choose a reason for hiding this comment

bergmeister Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

JamesWTruher Dec 6, 2018

Choose a reason for hiding this comment

bergmeister Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

rjmholt commented Dec 7, 2018

bergmeister commented Dec 15, 2018 • edited Loading

rjmholt commented Dec 17, 2018

bergmeister commented Nov 3, 2018 •

edited

Loading

bergmeister Dec 19, 2018 •

edited

Loading

JamesWTruher Dec 6, 2018 •

edited

Loading

bergmeister Dec 19, 2018 •

edited

Loading

bergmeister Dec 19, 2018 •

edited

Loading

bergmeister Dec 19, 2018 •

edited

Loading

bergmeister commented Dec 15, 2018 •

edited

Loading