X Tutup
The Wayback Machine - https://web.archive.org/web/20200918094833/https://github.com/PowerShell/PSScriptAnalyzer/pull/1092
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CheckInnerBrace and CheckPipe options to PSUseConsistentWhitespace #1092

Conversation

@bergmeister
Copy link
Collaborator

bergmeister commented Nov 3, 2018

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

bergmeister added 3 commits Nov 3, 2018
@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Nov 6, 2018

@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.

@JamesWTruher

JamesWTruher Dec 6, 2018 Member

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

This comment has been minimized.

@bergmeister

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.

@JamesWTruher

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.

@bergmeister

bergmeister Dec 19, 2018 Author Collaborator

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


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

This comment has been minimized.

@JamesWTruher

JamesWTruher Dec 6, 2018 Member

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

This comment has been minimized.

@bergmeister

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.

@JamesWTruher

JamesWTruher Dec 6, 2018 Member

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

This comment has been minimized.

@bergmeister

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.

@JamesWTruher

JamesWTruher Dec 6, 2018 Member

can these loops be combined in some way?

This comment has been minimized.

@bergmeister

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.

@rjmholt
Copy link
Member

rjmholt commented Dec 7, 2018

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
Copy link
Collaborator Author

bergmeister commented Dec 15, 2018

@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
Copy link
Member

rjmholt commented Dec 17, 2018

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

bergmeister added 2 commits Dec 19, 2018
…e the closing brace would not be correctly replaced
bergmeister added 3 commits Dec 19, 2018
…d test case for that
…er method to inform about limited functionality
bergmeister added 3 commits Jan 9, 2019
…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe
…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe

# Conflicts:
#	Rules/Strings.resx
…nalyzer into UseConsistenWhiteSpace_InnerCurlyAndPipe
# Conflicts:
#	Tests/Engine/InvokeScriptAnalyzer.tests.ps1
@rjmholt
rjmholt approved these changes Mar 5, 2019
@bergmeister bergmeister merged commit 484f405 into PowerShell:development Mar 5, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
bergmeister added a commit to bergmeister/PSScriptAnalyzer that referenced this pull request Mar 22, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.
X Tutup