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

PSUseConsistentIndentation: Check indentation of lines where first token is a LParen not followed by comment or new line #1995

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

liamjpeters
Copy link
Contributor

@liamjpeters liamjpeters commented Apr 12, 2024

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:

case TokenKind.LParen:
// When a line starts with a parenthesis and it is not the last non-comment token of that line,
// then indentation does not need to be increased.
if ((tokenIndex == 0 || tokens[tokenIndex - 1].Kind == TokenKind.NewLine) &&
NextTokenIgnoringComments(tokens, tokenIndex)?.Kind != TokenKind.NewLine)
{
onNewLine = false;
lParenSkippedIndentation.Push(true);
break;
}
lParenSkippedIndentation.Push(false);
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);
break;

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.

  • In the current 1.22.0 we find 2,927 violations.
  • With this PR applied, we find 2,931 violations.
  • I believe the settings I used (below) means the rule defaults to spaces and many of the files were using tabs - hence the large number of violations.
    $Settings = @{
        IncludeRules = @('PSUseConsistentIndentation')
        Rules        = @{
            PSUseConsistentIndentation = @{
                Enable = $true
            }
        }
    }
  • All previously found violations were still found.
  • There were 4 newly found violations:
    • \Tests\Engine\ModuleHelp.Tests.ps1\ModuleHelp.Tests.ps1 line 48

      if ((-not ((($commandModuleName = $CommandInfo.Module.Name) -and ($commandVersion = $CommandInfo.Module.Version)) -or
      (($commandModuleName = $CommandInfo.PSSnapin) -and ($commandVersion = $CommandInfo.PSSnapin.Version))))) {
      Write-Error "For $($CommandInfo.Name) : Can't find PSSnapin/module name and version"
      }
      else {
      # "For $commandName : Module is $commandModuleName. Version is $commandVersion"
      [PSCustomObject]@{ CommandName = $CommandInfo.Name; ModuleName = $commandModuleName; Version = $commandVersion }
      }

      This becomes:

      image

      Which is a little dubious 🤔

      Edit: This is because the previous line has 6 opening LParens, and 3 closing RParens. So the indentation level is 3 greater than the previous line

    • \Tests\Engine\ModuleHelp.Tests.ps1\ModuleHelp.Tests.ps1 line 116

      It "gets example code from <CommandName>" -TestCases $testCases {
      ($Help.Examples.Example | Select-Object -First 1).Code | Should -Not -BeNullOrEmpty
      }

      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

      {
      # Checks nesting structures
      $nestingASTs = $asts | Where-Object {($PSItem.Extent.StartLineNumber -gt $ast.Extent.StartLineNumber) -and
      ($PSItem.Extent.EndLineNumber -lt $ast.Extent.EndLineNumber)}
      # If one AST have end-of-line comments, we should skip it.
      [bool]$needComment = $ast.Extent.EndScriptPosition.Line.Trim().EndsWith("}")

      This is changed to:

      image

    • \Tests\Rules\UseToExportFieldsInManifest.tests.ps1\UseToExportFieldsInManifest.tests.ps1 line 73

      This is a line that randomly uses tabs

      image

PR Checklist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to format document in special circumstances
1 participant