Skip to content
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

Refactor GetCommandHandler to not use dynamic #1268

Merged
merged 2 commits into from
Apr 23, 2020
Merged

Refactor GetCommandHandler to not use dynamic #1268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b4cb7
Refactor GetCommandHandler to not use dynamic
TylerLeonhardt Apr 21, 2020
f6a2788
streamline a couple CommandType's
TylerLeonhardt Apr 23, 2020
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
36 changes: 30 additions & 6 deletions src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,29 +48,53 @@ public async Task<List<PSCommandMessage>> Handle(GetCommandParams request, Cance
PSCommand psCommand = new PSCommand();

// Executes the following:
// Get-Command -CommandType Function,Cmdlet,ExternalScript | Select-Object -Property Name,ModuleName | Sort-Object -Property Name
// Get-Command -CommandType Function,Cmdlet,ExternalScript | Sort-Object -Property Name
psCommand
.AddCommand("Microsoft.PowerShell.Core\\Get-Command")
.AddParameter("CommandType", new[] { "Function", "Cmdlet", "ExternalScript" })
.AddCommand("Microsoft.PowerShell.Utility\\Select-Object")
.AddParameter("Property", new[] { "Name", "ModuleName" })
.AddCommand("Microsoft.PowerShell.Utility\\Sort-Object")
.AddParameter("Property", "Name");

IEnumerable<PSObject> result = await _powerShellContextService.ExecuteCommandAsync<PSObject>(psCommand).ConfigureAwait(false);
IEnumerable<CommandInfo> result = await _powerShellContextService.ExecuteCommandAsync<CommandInfo>(psCommand).ConfigureAwait(false);

var commandList = new List<PSCommandMessage>();
if (result != null)
{
foreach (dynamic command in result)
foreach (CommandInfo command in result)
{
// Some info objects have a quicker way to get the DefaultParameterSet. These
// are also the most likely to show up so win-win.
string defaultParameterSet = null;
switch (command)
Copy link
Member Author

Choose a reason for hiding this comment

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

I just wanna use the new pattern matching switch but it says it's still "preview" 😭

Copy link
Member Author

Choose a reason for hiding this comment

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

Codacy Issue found: Add a 'default' clause to this 'switch' statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Codacy Issue found: Replace this 'switch' statement with 'if' statements to increase readability.

{
case CmdletInfo info:
defaultParameterSet = info.DefaultParameterSet;
break;
case FunctionInfo info:
defaultParameterSet = info.DefaultParameterSet;
break;
}

if (defaultParameterSet == null)
{
// Try to get the default ParameterSet if it isn't streamlined (ExternalScriptInfo for example)
foreach (CommandParameterSetInfo parameterSetInfo in command.ParameterSets)
{
if (parameterSetInfo.IsDefault)
{
defaultParameterSet = parameterSetInfo.Name;
break;
}
}
}

commandList.Add(new PSCommandMessage
{
Name = command.Name,
ModuleName = command.ModuleName,
Parameters = command.Parameters,
ParameterSets = command.ParameterSets,
DefaultParameterSet = command.DefaultParameterSet
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the story on needing the for-loop instead of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

CommandInfo's only have:

https://github.com/PowerShell/PowerShell/blob/b1e998046e12ebe5da9dee479f20d479aa2256d7/src/System.Management.Automation/engine/CommandInfo.cs#L611

no DefaultParameterSet

Copy link
Member Author

Choose a reason for hiding this comment

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

DefaultParameterSet's might only exist on CmdletInfo and FunctionInfo.. so I could special case those types... but there are a lot of *Info's

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we're probably mostly going to deal with those types, I think that would make sense as an optimisation

Copy link
Member Author

Choose a reason for hiding this comment

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

added a switch but still kept the loop for everything else (which atm is just ExternalScript).

DefaultParameterSet = defaultParameterSet
});
}
}
Expand Down
4 changes: 2 additions & 2 deletions test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,8 +1004,8 @@ await LanguageClient.SendRequest<EvaluateResponseBody>(
[Fact]
public async Task CanSendGetCommandRequest()
{
List<PSCommandMessage> pSCommandMessages =
await LanguageClient.SendRequest<List<PSCommandMessage>>("powerShell/getCommand", new GetCommandParams());
List<object> pSCommandMessages =
await LanguageClient.SendRequest<List<object>>("powerShell/getCommand", new GetCommandParams());

Assert.NotEmpty(pSCommandMessages);
// There should be at least 20 commands or so.
Expand Down