| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…at LParen doesn't impact the indentation level
…rst token on a line, followed by a non-newline, non-comment
There was a problem hiding this comment.
Thorough testing of formatting rules is always tricky although we've kept adding tests over the years. Have you done any manual smoke test with a build from this? If not, not an issue, we could always do a bit of that before release time
Sorry, something went wrong.
|
I've used it across several large code bases at work. I'm happy with the corrections it's suggested on those. Alongside the test run I did on the PSScriptAnalyzer module, I've tested it on a public module I've used a bit recently, IntuneManagement. It has a fairly large PowerShell codebase. Without this PR applied, using settings to limit analysis to just the rule in question, PSUseConsistentIndentation, I get 3,304 violations. With this PR applied I get 3,305. Just like the PSScriptAnalyzer module anyalysis, all previously found violations were still found. So there was 1 additional violation found. It's here on line 1,394. New PR would fix it to be: This is because Two Left-Parens are on line 1,393, with just the one Right-Paren. So the indentation on the following line is increased by 1. To me it reads slightly clearer as I scan down the code-block: if statement, indentation, left curly brace for the body of the statement. I'd personally probably format the code-block like: if (
($obj.PSObject.Properties | Where-Object Name -EQ 'securityRequireSafetyNetAttestationBasicIntegrity') -and
($obj.PSObject.Properties | Where-Object Name -EQ 'securityRequireSafetyNetAttestationCertifiedDevice')
) {But that's personal preference. Where's no sort of definitive style guide that covers this scenario (that I'm aware of anyway), it's not clear cut what should be done here. Ultimately if this PR moves things in a 'more correct' direction (whatever 'correct' is), then I'd say I'm happy that this doesn't break anything. But if you think this change is moving the other way - I won't be offended with it being rejected or some changes being suggested; it's all good 😀. |
Sorry, something went wrong.
|
This looks better to me too. Thanks for the thorough field testing. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
Currently if an LParen ( is the first token on a line and the next token is not a comment or new line, then the line's indentation is not checked.
This is due to this if-check:
PSScriptAnalyzer/Rules/UseConsistentIndentation.cs
Lines 165 to 177 in a754b95
AddViolation(), which subsequently checks the lines indentation against the expected indentation, is not called if the conditional evaluates to true.
This PR changes the logic to always call AddViolation(), so the indentation is checked, but to only increase the indentation level when the conditional evaluates to false.
Fixes #1994
I've run this rule recursively over some large code-bases, including the PSScriptAnalyzer repo.
\Tests\Engine\ModuleHelp.Tests.ps1\ModuleHelp.Tests.ps1 line 48
PSScriptAnalyzer/Tests/Engine/ModuleHelp.Tests.ps1
Lines 47 to 54 in a754b95
This becomes:
Which is a little dubious 🤔
\Tests\Engine\ModuleHelp.Tests.ps1\ModuleHelp.Tests.ps1 line 116
PSScriptAnalyzer/Tests/Engine/ModuleHelp.Tests.ps1
Lines 115 to 117 in a754b95
This line wasn't previously being checked. It's using tabs, and as mentioned above the settings is defaulting to spaces.
\Tests\Engine\CommunityAnalyzerRules\CommunityAnalyzerRules.psm1\CommunityAnalyzerRules.psm1 line 518
PSScriptAnalyzer/Tests/Engine/CommunityAnalyzerRules/CommunityAnalyzerRules.psm1
Lines 515 to 521 in a754b95
This is changed to:
\Tests\Rules\UseToExportFieldsInManifest.tests.ps1\UseToExportFieldsInManifest.tests.ps1 line 73
This is a line that randomly uses tabs
PR Checklist