Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd CheckInnerBrace and CheckPipe options to PSUseConsistentWhitespace #1092
Conversation
|
@bergmeister once this gets merged in, can you open an issue on vscode-powershell to add the settings? |
| $ruleConfiguration.CheckSeparator = $false | ||
| } | ||
|
|
||
| It "Should find a violation if there is no space after pipe" { |
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Dec 6, 2018
Member
Could these 2 be simplified into Get-Item|foo and look for 2 errors?
This comment has been minimized.
This comment has been minimized.
bergmeister
Dec 19, 2018
•
Author
Collaborator
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.
| } | ||
|
|
||
| It "Should not find a violation if there is 1 space before and after a pipe" { | ||
| $def = 'Get-Item | foo' |
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Dec 6, 2018
•
Member
should we be checking for things like Get-Item | foo? (there are more than one space in there)
This comment has been minimized.
This comment has been minimized.
|
|
||
| It "Should not find a violation if there is 1 space inside empty curly braces" { | ||
| $def = @' | ||
| if($true) { } |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bergmeister
Dec 19, 2018
Author
Collaborator
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
| { | ||
| if (lCurly.Next == null | ||
| || !IsPreviousTokenOnSameLine(lCurly) | ||
| || lCurly.Next.Value.Kind == TokenKind.LCurly |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bergmeister
Dec 19, 2018
•
Author
Collaborator
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
| } | ||
| } | ||
|
|
||
| foreach (var pipe in tokenOperations.GetTokenNodes(TokenKind.Pipe)) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bergmeister
Dec 19, 2018
•
Author
Collaborator
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.
|
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. |
|
@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 |
|
Ok good, just making sure these didn't meet the proviso of my comments. In that case I'm happy with everything |
# 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

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

bergmeister commentedNov 3, 2018
•
edited
PR Summary
Closes #1065 and #1113 by adding 2 new rule options (
CheckInnerBraceandCheckPipe) toPSUseConsistentWhitespacethat 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.codeFormattingsettings to the vs code extension that control those 2 new options. cc @TylerLeonhardt @rjmholtPR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.