-
Notifications
You must be signed in to change notification settings - Fork 223
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
Refactor GetCommandHandler to not use dynamic #1268
Refactor GetCommandHandler to not use dynamic #1268
Conversation
commandList.Add(new PSCommandMessage | ||
{ | ||
Name = command.Name, | ||
ModuleName = command.ModuleName, | ||
Parameters = command.Parameters, | ||
ParameterSets = command.ParameterSets, | ||
DefaultParameterSet = command.DefaultParameterSet |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CommandInfo's only have:
no DefaultParameterSet
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
// 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) |
There was a problem hiding this comment.
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" 😭
Here is an overview of what got changed by this pull request: Issues
======
- Added 2
Complexity increasing per file
==============================
- src/PowerShellEditorServices/Services/PowerShellContext/Handlers/GetCommandHandler.cs 3
See the complete overview on Codacy |
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue found: Add a 'default' clause to this 'switch' statement.
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor GetCommandHandler to not use dynamic.
This includes a test change that was needed because a PowerShell type didn't serialize correctly.
cc @corbob