PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.85k stars 373 forks source link

Whitespace removed when "Code Formatting: Whitespace Around Operator" Enabled #1778

Open EmeraldFlame opened 2 years ago

EmeraldFlame commented 2 years ago

Prerequisites

Summary

When enabling the "Whitespace Around Operator" formatting option, which description reads "Adds spaces before and after an operator...", the setting will also remove redundant whitespace around operators, despite the description not detailing that.

This means that if a user deliberately adds additional whitespace in order to line up declarations, that deliberate extra whitespace is removed and their declarations no longer cleanly line up.

From my perspective, the removal of the additional whitespace should be moved to it's own separate preference, which is how it already operates for "Add Whitespace Around Pipe" and "Trim Whitespace Around Pipe". For these settings related to pipes, the Add setting only adds, and the Trim setting only removes. Or at the very least, the dual nature of "Whitespace Around Operator" should be detailed in it's description.


While not directly related to the bug itself, I could also very easily see an argument being made to have an option similar to "Align Property Value Pairs" when multiple variable declarations are made on consecutive lines, like in the instance of my example below. At least for my personal preference a setting like that could also be an reasonable solution to this problem.

PowerShell Version

Name                           Value
----                           -----
PSVersion                      7.2.1
PSEdition                      Core
GitCommitId                    7.2.1
OS                             Microsoft Windows 10.0.19042
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}       
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Visual Studio Code Version

1.64.2
f80445acd5a3dadef24aa209168452a3d97cc326
x64

Extension Version

ms-vscode.powershell@2021.12.0

Steps to Reproduce

  1. Enable the "Code Formatting: Whitespace Around Operator" preference
  2. Write code with multiple spaces between variable name and operator
  3. Apply formatting

Visuals

Input

$foo     = 'bar'
$testing = 'stuff'
$month='March'

Expected behavior (based on description text that only mentions adding whitespace)

$foo     = 'bar'
$testing = 'stuff'
$month = 'March'

Actual behavior

$foo = 'bar'
$testing = 'stuff'
$month = 'March'

Logs

No response

bergmeister commented 2 years ago

Operators are a lot of things, not just = but also +, etc. therefore for the use case of aligning equals might warrant its own setting. One needs to make a compromise between offering full granularity and the amount of settings for the user to read/understand therefore I am not sure if this use case is common enough. Also, even if we did add this setting, then whilst it would add a whitespace where there is none, you still have to manually indent every line to achieve your goal of aligning your = signs vertically. If you want to vertically align a set of variables and let the editor do that for you, then I suggest you define the variables as a hashtable as PSSA will then align the equals signs for you. Example:

$variableBagOfSomething = @{
    $foo = 'bar'
    $testing = 'stuff'
    $month='March'
}

After the format it looks like

$variableBagOfSomething = @{
    $foo     = 'bar'
    $testing = 'stuff'
    $month   = 'March'
}

What do you think, wouldn't that solve your use case better without having to add those granular settings? @StevenBucher98

EmeraldFlame commented 2 years ago

Aside

As I mentioned in my original post, I personally think the auto-align functionality could be an interesting and useful addition, and this bug highlighted that thought for me, but I want to make it clear, that the auto align = is not actually the reason I opened the issue. I do still however think it's compelling, at least to me. While your hashtable option does work and I could change my workflow to fit it, there are a number of times I've wanted to do this simply because it improves legibility, but the actual variables aren't related in any way, so from a logical structure perspective, it doesn't necessarily make sense to group them.

But, let's redirect away from that. I'll consider opening a separate issue as a feature request, and likely should have done that to begin with instead of mentioning it here as it seems to be detracting from the actual issue. Thinking about this bug just naturally led my brain there, but it's caused I think 2 separate discussions which wasn't my goal.

Actual issue

Back to what I consider the actual bug: my biggest issue with the way this setting works is it takes very specific actions that it does not detail in its description, while also behaving differently from other similar options.

To compare, here is the full name and description of two rules that from the way their descriptions are written, sound like they should behave similarly:

  • Whitespace Around Operator: Adds spaces before and after an operator ('=', '+', '-', etc)
  • Add Whitespace Around Pipe: Adds a space before and after the pipeline operator ('|') if it is missing

Both of these descriptions only mention adding whitespace, neither mention removing whitespace. However, Around Operator does remove whitespace and Around Pipe does not. So the options seems inconsistent between each other.

To visualize, lets say we have both options mentioned turned on and go through the following scenario:

Example Scenario

Input

#0 spaces
$a=1+7
Get-Stuff|Write-Stuff
#1 space
$b = 2 + 8
Get-MoreStuff | Write-MoreStuff
#5 spaces
$c     =     3     +     9
Get-AllStuff     |     Write-AllStuff

Expected Output Based on the descriptions only mentioning adding, my expected output would be:

#0 spaces
$a = 1 + 7
Get-Stuff | Write-Stuff
#1 space
$b = 2 + 8
Get-MoreStuff | Write-MoreStuff
#5 spaces
$c     =     3     +     9
Get-AllStuff     |     Write-AllStuff

Actual Output The actual output is what highlights the inconsistency I'm talking about and my personal frustration with the setting, notice the difference in handling between the math operators and the pipes in the 5 space section

#0 spaces
$a = 1 + 7
Get-Stuff | Write-Stuff
#1 space
$b = 2 + 8
Get-MoreStuff | Write-MoreStuff
#5 spaces
$c = 3 + 9
Get-AllStuff     |     Write-AllStuff

To remove the extra whitespace around the pipe characters, you additionally need to enable the rule

Trim Whitespace Around Pipe: Trims extraneous whitespace (more than 1 character) before and after the pipeline operator ('|')

I ultimately feel that the rest of the operators should behave in the same manner and have 2 separate rules just like pipeline operators do. One rule to add space, and another rule to remove it, that way end users can make all their operators behave the same whether they are math, pipeline, etc. I think adding that 1 additional option would be the correct solution here. However if for some reason that can't happen, I think re-wording the description for Around Operator so that it's clear that it not only adds whitespace but also removes redundant spaces is an absolute must.

TL:DR: Ideally I think whitespace around the math operators (and others) should split into two options, 1 for add whitespace, 1 for trim whitespace, so that it is consistent with how whitespace around pipeline operators are already handled.