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

TylerLeonhardt · 2018-01-12T04:25:12Z

This works both in local and remote sessions.

psedit became Open-EditorFile
and psedit aliases Open-EditorFile

Resolves #528

rkeithhill · 2018-01-12T04:50:29Z

module/PowerShellEditorServices/Commands/PowerShellEditorServices.Commands.psd1

@@ -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 = @()


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. :-)

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

Thanks!

rkeithhill · 2018-01-12T04:59:44Z

src/PowerShellEditorServices/Session/RemoteFileManager.cs

+                Remove-Item -Path 'function:\global:Open-EditorFile' -Force
+            }
+
+            if (Get-Alias psedit)


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?

rkeithhill · 2018-01-12T05:03:42Z

src/PowerShellEditorServices/Session/RemoteFileManager.cs

+
+            if (Get-Alias psedit)
+            {
+                Remove-Alias psedit


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.

SeeminglyScience · 2018-01-12T18:18:10Z

module/PowerShellEditorServices/Commands/Public/CmdletInterface.ps1

@@ -77,11 +77,13 @@ function Unregister-EditorCommand {
    }
 }

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

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


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 {

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

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

daviwil

Agree with Keith's recommendations. Other than that, looks good!

SeeminglyScience · 2018-01-12T20:16:07Z

src/PowerShellEditorServices/Session/RemoteFileManager.cs

+
+            if (Test-Path -Path 'alias:\psedit')
+            {
+                Remove-Item -Path 'alias:\psedit' -Force
            }

            Get-EventSubscriber -SourceIdentifier PSESRemoteSessionOpenFile -EA Ignore | Remove-Event


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

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

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

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.

SeeminglyScience · 2018-01-12T21:13:01Z

src/PowerShellEditorServices/Session/RemoteFileManager.cs

@@ -74,16 +74,22 @@ public class RemoteFileManager

            Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile {0}


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.

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

That said, I can change:

Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile {0}

to

Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile -Forward

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

-Forward added :)

rkeithhill · 2018-01-12T21:37:49Z

module/PowerShellEditorServices/Commands/PowerShellEditorServices.Commands.psd1

@@ -77,7 +77,8 @@ FunctionsToExport = @('Register-EditorCommand',
                      'Out-CurrentFile',
                      'Join-ScriptExtent',
                      'Test-ScriptExtent',
-                      'psedit')
+                      'psedit',


There is an AliasesToExport field down below to put psedit in. That is where it should go.

Doh! Fixed now.

change psedit to Open-EditorFile and alias psedit to it

ea965c2

TylerLeonhardt requested review from daviwil, rkeithhill and SeeminglyScience January 12, 2018 04:25

rkeithhill reviewed Jan 12, 2018

View reviewed changes

SeeminglyScience reviewed Jan 12, 2018

View reviewed changes

daviwil approved these changes Jan 12, 2018

View reviewed changes

address misc feedback

efaf129

SeeminglyScience reviewed Jan 12, 2018

View reviewed changes

remove -> unregister

c447d84

SeeminglyScience reviewed Jan 12, 2018

View reviewed changes

SeeminglyScience approved these changes Jan 12, 2018

View reviewed changes

rkeithhill reviewed Jan 12, 2018

View reviewed changes

misc feedback

e4516c2

rkeithhill approved these changes Jan 12, 2018

View reviewed changes

TylerLeonhardt merged commit dafbbc7 into PowerShell:master Jan 16, 2018

TylerLeonhardt deleted the open-editorfile branch January 16, 2018 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TylerLeonhardt commented Jan 12, 2018 •

edited

Loading

rkeithhill Jan 12, 2018

TylerLeonhardt Jan 12, 2018

TylerLeonhardt Jan 12, 2018

rkeithhill Jan 12, 2018

TylerLeonhardt Jan 12, 2018

rkeithhill Jan 12, 2018

TylerLeonhardt Jan 12, 2018

SeeminglyScience Jan 12, 2018

rkeithhill Jan 12, 2018

TylerLeonhardt Jan 12, 2018

daviwil left a comment

SeeminglyScience Jan 12, 2018

daviwil Jan 12, 2018

SeeminglyScience Jan 12, 2018

TylerLeonhardt Jan 12, 2018

TylerLeonhardt Jan 12, 2018

SeeminglyScience Jan 12, 2018

TylerLeonhardt Jan 12, 2018

TylerLeonhardt Jan 12, 2018

SeeminglyScience Jan 12, 2018

TylerLeonhardt Jan 12, 2018

rkeithhill Jan 12, 2018

TylerLeonhardt Jan 12, 2018

		@@ -74,16 +74,22 @@ public class RemoteFileManager

		Register-EngineEvent -SourceIdentifier PSESRemoteSessionOpenFile {0}

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

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

Conversation

TylerLeonhardt commented Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviwil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Jan 12, 2018 •

edited

Loading