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

PSUseConsistentWhitespace: Handle redirect operators which are not in stream order #2001

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liamjpeters
Copy link
Contributor

PR Summary

When CheckParameter is $true for the rule PSUseConsistentWhitespace, it checks whitespace between parameters by inspecting the AST. It gets all direct children of each CommandAst and checks, in order, that each's extent is separated by exactly 1 character.

This relies on the implicit assumption that the AST returns the children in something resembling token order.

In the case of redirect operators, this is not always the case (they anecdotally appear to be returned in the order of the stream they're redirecting). See the issue here for more detailed breakdown.

This PR adds sorting of the children before subsequently performing the same checks on the tokens extents.

Running the below across PS5.1 and PS7.4 with and without the PR change applied shows the performance impact of the change to be minimal.

Measure-Command {
    foreach ($i in 1..10000) {
        Invoke-Formatter -ScriptDefinition "Invoke-Foo $i 3>&1 2>&1 1>`$null" -Settings @{
            Rules = @{
                PSUseConsistentWhitespace = @{
                    Enable         = $true
                    CheckParameter = $true
                }
            }
        }
    }
}
PS5.1 Pre-PR PS5.1 Post-PR PS7.4.2 Pre-PR PS7.4.2 Post-PR
Invoke-Formatter 231.3535187 156.0559906 16.8896873 13.4352551
Invoke-ScriptAnalyzer 253.1291937 250.9393157 21.8151956 19.8125177

The 'speedup' is presumably that we're no longer emitting a diagnostic record

Fixes #2000

PR Checklist

@bergmeister
Copy link
Collaborator

Awesome @liamjpeters . I will try to take some time soon to review this and all your other good work that I've glanced over. Thanks for your patience in the meantime, definitely excited to get your changes in.

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.

whitespaceBetweenParameters Removing Redirectors
2 participants