AlignAssignmentStatement overhaul to fix issues and include handing of Enums.
PR Summary
Overhauls the AlignAssignmentStatement rule to include handling of enums as well as fix several issues with Hashtable alignment.
I am very open to feedback and suggestions 😀.
Setting defaults have changed. Previously the rule could be enabled, but not do anything, as CheckHashtable (previously the only setting and gating all functionality) defaulted to $false. I've changed this (as well as the new settings) to default to $true. If you enable the rule - it will do something now. You can then optionally disable any parts you don't want. The rule is opt-in and there are already breaking changes in this PR (see the baby and the bath water below). Docs update shows and explains new settings.
Hashtable alignment issues resolved include:
-
Erroring when the key of a key-value pair is an expression containing an equals sign
@{ ($Key1 = 5) = "Value1" Key2 = "Value2" }Invoke-ScriptAnalyzer: Start position cannot be before End position.This is because of the way the current implementation finds
Equalstokens. It find the equals token within the expression.It results in a correction extent that has an end column which is before the start column.
-
Erasing inline comments when they appear between the key of the key-value pair and the
Equalstoken.@{ Key1 <#Sneakycomment#> = "Value1" Key2 = "Value2" }results in
@{ Key1 = "Value1" Key2 = "Value2" } -
Performance scaling poorly. This is due to how
Equalstokens of Key-Value pairs are located. For each Key-Value pair the list of tokens is scanned (from the beginning) until the relevantEqualstoken is found. This means that, as a worst case, performance scales quadratically - proportional to half the number of Key-Value pairs in the file * the number of tokens.We can improve this by scanning the tokens once and keeping track of all the
Equalstokens and what line they appear on. This reduces complexity to essentially linear time with negligible memory use. Goes fromO(H*T)->O(H+T).I've put together some stress-test files (one saved as a gist here and script to generate them here) and ran only the
AlignAssignmentStatementrule on them. Results below from my machine, averaged over 5 runs (with the first cold run discarded):Violations Pre-PR Post-PR 100 Hashtables with 100 KVP 8,301 3.627s 0.221s 500 Hashtables with 100 KVP 41,124 121.902s 1.533s 1000 Hashtables with 100 KVP 82,147 457.873s 2.165s -
The baby is thrown out with the bath water. If a single Key-Value pair is not in the expected format (Key and Equals-sign on same line, no two kvp on same line etc) the whole hashtable is skipped. I don't think this should be the case. The rule should consider and align the keys which are in the expected format and ignore the others. The rule is opt-in and so people using it want alignment - we should do our best to provide it.
I've done my best to expand the test coverage.
Fixes #1739 Fixes #1860
Would address downstream VSCode Extension issue: https://github.com/PowerShell/vscode-powershell/issues/5138
PR Checklist
- [x] PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
- [x] Summarized changes
- [ ] Change is not breaking
- [x] Make sure all
.cs,.ps1and.psm1files have the correct copyright header - [x] Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
- [x] This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix
WIP:to the beginning of the title and remove the prefix when the PR is ready.
- If the PR is work in progress, please add the prefix
This is great! I won't pretend to know what you did regarding performance (I'm merely a PowerShell coder, not a C# programmer), but was hoping enums would be added to the PSAlignAssignmentStatement rule, so kudos! I just wrote a custom rule to handle this very issue, but there's no way it performs as well and strips comments.
This is great! I won't pretend to know what you did regarding performance (I'm merely a PowerShell coder, not a C# programmer) ...
👋 @TMA-2
I'm also merely a PowerShell coder, not a C# programmer day-to-day 😀 - I'm sure it shows to those that are - but I'm interested and am giving it a go.
There's a lot to this PR, some desirable - some perhaps not 😅. I imagine it will be a while yet before anyone has the time to review it, and it may need to change quite a bit once it is.
This is great! I won't pretend to know what you did regarding performance (I'm merely a PowerShell coder, not a C# programmer) ...
👋 @TMA-2
I'm also merely a PowerShell coder, not a C# programmer day-to-day 😀 - I'm sure it shows to those that are - but I'm interested and am giving it a go.
There's a lot to this PR, some desirable - some perhaps not 😅. I imagine it will be a while yet before anyone has the time to review it, and it may need to change quite a bit once it is.
Certainly don't mean to downplay PowerShell -- I can't get enough, personally, despite all the gaps in the ecosystem that don't seem to exist with other languages. But nonetheless, lots of ingenious things come from the community that shouldn't seem possible (PODE alone is amazing!).
Here's hoping this gets a review quite soon. There's still an open PR for fixing unary operator formatting from June 17 with no comments, so the maintainers may be busy with other projects.
I could still attempt to take a look and see what I see, for what it's worth?