Skip to content

Fix Import-EditorCommand bug #1114

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.

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 2 commits into from
Nov 26, 2019
Merged

Fix Import-EditorCommand bug #1114

merged 2 commits into from
Nov 26, 2019

Conversation

sk82jack
Copy link
Contributor

It appears that PR #1056 involved renaming the extensionService field to _extensionService but this wasn't changed in the Import-EditorCommand function causing an error when trying to use it.

Following a discussion with @TylerLeonhardt on the PowerShell Discord server I've submitted this PR to fix the error by updating the field name in Import-EditorCommand.

I didn't see any tests or anything that needed to be changed but let me know if you need me to do anything else in order for this to be accepted 🙂

@sk82jack sk82jack requested a review from rjmholt as a code owner November 24, 2019 23:53
@@ -59,7 +59,7 @@ function Import-EditorCommand {
}
$flags = [Reflection.BindingFlags]'Instance, NonPublic'
$extensionService = $psEditor.GetType().
GetField('extensionService', $flags).
GetField('_extensionService', $flags).
GetValue($psEditor)

$editorCommands = $extensionService.GetType().
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually you can replace all of these with $editorCommands = $psEditor.GetCommands(). That API didn't exist at the time, but now that it does that should definitely be used instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah that looks a lot nicer. I can change it to that but I'm not sure about the correct procedure in terms of not cluttering up the commit history - should I do an interactive rebase to clean up or leave it up to the maintainer to squash & merge?

Copy link
Collaborator

@SeeminglyScience SeeminglyScience Nov 25, 2019

Choose a reason for hiding this comment

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

leave it up to the maintainer to squash & merge?

That one :)

Also taking a closer look you'll probably want to do something like:

$editorCommands = [System.Collections.Generic.Dictionary[string, Microsoft.PowerShell.EditorServices.Services.PowerShellContext.EditorCommand]]::new(
    [System.StringComparer]::OrdinalIgnoreCase)

foreach ($existingCommand in $psEditor.GetCommands()) {
    $editorCommands.Add($existingCommand.Name, $existingCommand)
}

Optionally add some using namespace statements to clean it up a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointers - will get that implemented and confirm it all still works

Out of interest why do you suggest a strongly typed dictionary over a regular hashtable? Performance or just clearer intent/more control or something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I was just matching the type of the field, hashtable will probably work just fine and be a bit more clean. Good call :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome. Just pushed new version 🙂

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

No more reflection 🎉 LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants