Skip to content

change psedit to Open-EditorFile and alias psedit to it #609

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 16, 2018
Merged

change psedit to Open-EditorFile and alias psedit to it #609

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea965c2
change psedit to Open-EditorFile and alias psedit to it
TylerLeonhardt Jan 12, 2018
efaf129
address misc feedback
TylerLeonhardt Jan 12, 2018
c447d84
remove -> unregister
TylerLeonhardt Jan 12, 2018
e4516c2
misc feedback
TylerLeonhardt Jan 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion module/PowerShellEditorServices/Commands/PowerShellEditorServices.Commands.psd1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ FunctionsToExport = @('Register-EditorCommand',
'Out-CurrentFile',
'Join-ScriptExtent',
'Test-ScriptExtent',
'psedit')
'Open-EditorFile')

# Cmdlets to export from this module, for best performance, do not use wildcards and do not delete the entry, use an empty array if there are no cmdlets to export.
CmdletsToExport = @()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be an entry to export psedit as an alias? I guess for this module we don't need to worry about module auto-loading (since it is pre-loaded into PSIC). Still, somehow feels better to declare everything in the manifest. :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting! didn't know you needed to list aliases in FunctionsToExport.

Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

Expand Down
6 changes: 4 additions & 2 deletions module/PowerShellEditorServices/Commands/Public/CmdletInterface.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,13 @@ function Unregister-EditorCommand {
}
}

function psedit {
function Open-EditorFile {
param([Parameter(Mandatory=$true)]$FilePaths)

dir $FilePaths | where { !$_.PSIsContainer } | % {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're modifying this function, it may be a good time to replace the aliases and use the -File parameter (added in PSv3)

Get-ChildItem $FilePaths -File | ForEach-Object {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. PSSA should have flagged these inside VSCode. Hmm....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did :) I just missed it. Fixed this!

$psEditor.Workspace.OpenFile($_.FullName)
}
}
Export-ModuleMember -Function psedit
Set-Alias psedit Open-EditorFile -Scope Global

Export-ModuleMember -Function Open-EditorFile
14 changes: 10 additions & 4 deletions src/PowerShellEditorServices/Session/RemoteFileManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,22 @@ public class RemoteFileManager

Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile {0}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to pollute your PR, but I found another one. The input for {0} is passed on line 494. It either passes string.Empty for local runspaces or -Forward for remote runspaces. Register-EngineEvent requires either the -Forward switch or the -Action script block parameter.

Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile
# Register-EngineEvent : Action must be specified for non-forwarded events.
# At line:1 char:1
# + Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile
# + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#     + CategoryInfo          : InvalidArgument: (:) [Register-EngineEvent], ArgumentException
#     + FullyQualifiedErrorId : ACTION_MANDATORY_FOR_LOCAL,Microsoft.PowerShell.Commands.RegisterEngineEventCommand

I'm thinking we probably just need to always pass -Forward. This may explain psedit not working properly with some session types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may explain psedit not working properly with some session types.

which session types? I haven't had any issue with WinRM, SSH, or PowerShell Direct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, I can change:

Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile {0}

to

Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile -Forward

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tylerl0706 ah right, the condition is probably never hit. It should still be changed at some point, but I doubt it actually affects anything.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-Forward added :)


if ((Test-Path -Path 'function:\global:PSEdit') -eq $false)
if ((Test-Path -Path 'function:\global:Open-EditorFile') -eq $false)
{{
Set-Item -Path 'function:\global:PSEdit' -Value $PSEditFunction
Set-Item -Path 'function:\global:Open-EditorFile' -Value $PSEditFunction
Set-Alias psedit Open-EditorFile -Scope Global
}}
";

private const string RemovePSEditFunctionScript = @"
if ((Test-Path -Path 'function:\global:PSEdit') -eq $true)
if ((Test-Path -Path 'function:\global:Open-EditorFile') -eq $true)
{
Remove-Item -Path 'function:\global:PSEdit' -Force
Remove-Item -Path 'function:\global:Open-EditorFile' -Force
}

if (Get-Alias psedit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unlikely that psedit wouldn't be defined but if it isn't this will generate a non-term error. Maybe use Test-Path alias:\psedit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

{
Remove-Alias psedit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I believe Remove-Alias was added for PS Core. In my Win PS 5.1 that command doesn't exist.

remove-alias : The term 'remove-alias' is not recognized as the name of a cmdlet, function, script file, or operable
program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

Try Remove-Item alias:\psedit instead. That should work across all versions of PS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

}

Get-EventSubscriber -SourceIdentifier PSESRemoteSessionOpenFile -EA Ignore | Remove-Event
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR may not be the right place to change this, but are we sure Remove-Event actually works here? I feel like either that's supposed to be Unregister-Event or Get-EventSubscriber is supposed to be Get-Event

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I'm not sure, I stole this code from the ISE codebase :) If you think there's an actual problem here we should definitely fix it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile -Forward
Get-EventSubscriber -SourceIdentifier PSESRemoteSessionOpenFile | Remove-Event

# Remove-Event : The input object cannot be bound to any parameters for the command either because the command does not
# take pipeline input or the input and its properties do not match any of the parameters that take pipeline input.
# At line:1 char:65
# + ... ntSubscriber -SourceIdentifier PSESRemoteSessionOpenFile | Remove-Event
# +                                                              ~~~~~~~~~~~~
#     + CategoryInfo          : InvalidArgument: (System.Manageme...EventSubscriber:PSObject) [Remove-Event], ParameterB
#    indingException
#     + FullyQualifiedErrorId : InputObjectNotBound,Microsoft.PowerShell.Commands.RemoveEventCommand

@tylerl0706 Remove-Event should be Unregister-Event

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right :)

PS > Get-EventSubscriber | Remove-Event
Remove-Event : The input object cannot be bound to any parameters for the command either because the command does not take pipeline input or the input and its properties do not match any of the parameters that take pipeline input.
At line:1 char:23
+ Get-EventSubscriber | Remove-Event
+                       ~~~~~~~~~~~~
+ CategoryInfo          : InvalidArgument: (System.Manageme...EventSubscriber:PSObject) [Remove-Event], ParameterBindingException
+ FullyQualifiedErrorId : InputObjectNotBound,Microsoft.PowerShell.Commands.RemoveEventCommand

PS > Get-EventSubscriber | Unregister-Event
PS >

I'll change it to Unregister-Event.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

Expand Down