Open Razmo99 opened 3 months ago
Looks like there's some tests that need to be fixed after pulling in latest main
Hey @JustinGrote I've updated the tests to use Async, they should at least run / compile now
Latest MacOS/Ubuntu test failures look like a path combine issue.
System.IO.DirectoryNotFoundException : Could not find a part of the path '/Users/runner/work/PowerShellEditorServices/PowerShellEditorServices/test/PowerShellEditorServices.Test.Shared/Refactoring\Utilities/TestDetection.ps1'.
Note the backslash after Refactoring, may want to make sure you are using Path.Combine
or Join-Path
EDIT: Found the backslash hardcoded, refactored to use the three-argument path.combine, this should still work with net462
OK, all tests pass in CI now :). I'll do my best to get a first-pass review done sometime this week, and provide devcontainer/codespaces instructions to allow people to test for edge cases we need to document.
Looking pretty good so far in terms of design, some cleanup will be needed.
Several foreach rename issues I've already come across, I'll try to define tests for these when I get a chance.
#Doesnt rename testvar
$a = 1..5
$b = 6..10
function test {
process {
foreach ($testvar in $a) {
$testvar
}
foreach ($testvar in $b) {
$testvar
}
}
}
#Renames both foreach local variables ($aPath), should only rename the one in scope (agreed this is bad practice though)
function Import-FileNoWildcard {
[CmdletBinding(SupportsShouldProcess=$true)]
param(
# Specifies a path to one or more locations.
[Parameter(Mandatory=$true,
Position=0,
ParameterSetName="Path",
ValueFromPipeline=$true,
ValueFromPipelineByPropertyName=$true,
HelpMessage="Path to one or more locations.")]
[Alias("PSPath", "Path")]
[ValidateNotNullOrEmpty()]
[string[]]
$Path2
)
begin {
}
process {
# Modify [CmdletBinding()] to [CmdletBinding(SupportsShouldProcess=$true)]
$paths = @()
foreach ($aPath in $Path2) {
if (!(Test-Path -LiteralPath $aPath)) {
$ex = New-Object System.Management.Automation.ItemNotFoundException "Cannot find path '$aPath' because it does not exist."
$category = [System.Management.Automation.ErrorCategory]::ObjectNotFound
$errRecord = New-Object System.Management.Automation.ErrorRecord $ex,'PathNotFound',$category,$aPath
$psCmdlet.WriteError($errRecord)
continue
}
# Resolve any relative paths
$paths += $psCmdlet.SessionState.Path.GetUnresolvedProviderPathFromPSPath($aPath)
}
foreach ($aPath in $paths) {
if ($pscmdlet.ShouldProcess($aPath, 'Operation')) {
# Process each path
$aPath
}
}
}
end {
}
}
We will also need to make sure we account for the scoping issues as mentioned by Rob that are very difficult to handle, even if it means we warn about this as best effort, or straight up refuse to do it. https://github.com/PowerShell/vscode-powershell/issues/261#issuecomment-465249755
I made some test cases for renaming from within a for / each loop and limiting the rename to the scope if the var is defined within the creation of the statement. My only concern is a case like below.
If you run the code powershell treats the $i = 10
as the highest scope assignment which is then redefined in the loop but kept in function scope for when its printed. As it stands if you renamed $i = 10
it would rename the loop using the same variable. however if you renamed from within the loop it would not touch the $i=10
$a = 1..5
$b = 6..10
function test {
process {
$i = 10
for ($Renamed = 0; $Renamed -lt $b.Count; $Renamed++) {
$null = $Renamed
}
for ($i = 0; $i -lt $a.Count; $i++) {
write-output $i
}
write-output "I will be 5 : $i not 10"
}
}
test
PR Summary
This is a sister pull request to pr #4950 and issue Smart Variable Rename. It implements the server side component to allow refactoring of Powershell functions and variables.
It does this by implementing two new handlers
RenameSymbolHandler
andPrepareRenameSymbolHandler
which is just plumbing to receive the incoming requests from the client.PrepareRenameSymbolHandler
is used to determine if the rename is possible andRenameSymbolHandler
is what actually returns the edits to the client.All the action to decide what is renamed is done within two classes
IterativeFunctionRename
andIterativeVariableRename
to boil it down the Abstract Syntax Tree(AST) of the script file is analyzed, then the symbol at the target line and column is retrieved. It then loops through the AST following rules to determine scope and what to rename. This is then passed back to the client to action.Below are the current features
Below are the current limitations
Tests to cover as many scenarios as I could think of and to ensure the features described above function have been written
PR Context
This pull request should be helpful as its implementing a feature that I think has been desired for some time among Powershell developers. It should improve coding efficiency and allow code to be refactored with ease.