-
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
Signaturehelp cancellation and some caching #1251
Signaturehelp cancellation and some caching #1251
Conversation
I could add logging when it reads from the cache but then I'd need to pass in the logger. |
s_nounExclusionList.TryAdd("InstalledModule", true); | ||
s_nounExclusionList.TryAdd("InstalledScript", true); | ||
s_nounExclusionList.TryAdd("ScriptFileInfo", true); | ||
s_nounExclusionList.TryAdd("PSRepository", true); |
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.
According to the comment on line 62, shouldn't PowerShellGet
be in this list?
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.
Ah nevermind, this is the noun list, duh. Not the package name list.
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.
That said, I see more nouns from PowerShellGet
2.2.3 than appear in the list above. gcm -m PowerShellGet | % Noun | % Sort -uniq
Is that a problem? For instance, there is a Find-Command
function in PowerShellGet.
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.
Yeah I just renamed the variable. We should have a better check for verb-noun when PSGet uses the same noun as an inbox module Module
as an example.
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've added a s_cmdletExclusionList now which will handle that ^
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.
Changes LGTM!
One thing in existing code to consider, definitely not blocking.
{ | ||
signatures = new SignatureInformation[parameterSets.Signatures.Length]; | ||
for (int i = 0; i < signatures.Length; i++) | ||
var parameters = new ParameterInformation[parameterSets.Signatures[i].Parameters.Count()]; |
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.
Do we know why Parameters
is typed as IEnumerable<>
? I know this is existing code, but while you're here it might be worth changing that to an array and use Length
here instead of Count()
.
If it's IEnumerable<>
because it's a yield enumerator or LINQ then Count()
should be skipped and a List<>
with a best estimate capacity should be used.
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.
Not really sure what the best estimate could be here... so I'll just leave it empty.
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!
@@ -71,15 +70,13 @@ public async Task<SignatureHelp> Handle(SignatureHelpParams request, Cancellatio | |||
return new SignatureHelp(); | |||
} | |||
|
|||
SignatureInformation[] signatures = new SignatureInformation[parameterSets.Signatures.Length]; | |||
var signatures = new SignatureInformation[parameterSets.Signatures.Length]; |
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.
👍
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.
LGTM
Here is an overview of what got changed by this pull request: Issues
======
+ Solved 1
Complexity increasing per file
==============================
- src/PowerShellEditorServices/Services/TextDocument/Handlers/SignatureHelpHandler.cs 1
- src/PowerShellEditorServices/Services/PowerShellContext/Utilities/CommandHelpers.cs 2
Clones added
============
- src/PowerShellEditorServices/Services/TextDocument/Handlers/SignatureHelpHandler.cs 1
See the complete overview on Codacy |
CmdletInfo
's and synopsis' ofCmdletInfo
s.NounExclusionList
tos_nounExclusionList