-
Notifications
You must be signed in to change notification settings - Fork 234
Reduce allocations in the CodeLens providers #719
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
Changes from all commits
dccea93
d9d1d35
4d200bd
dc09c18
76d7aad
4136b56
4f7230f
66d976c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,71 +15,90 @@ namespace Microsoft.PowerShell.EditorServices.CodeLenses | |
{ | ||
internal class PesterCodeLensProvider : FeatureProviderBase, ICodeLensProvider | ||
{ | ||
private static char[] QuoteChars = new char[] { '\'', '"'}; | ||
/// <summary> | ||
/// The editor session context to provide CodeLenses for. | ||
/// </summary> | ||
private EditorSession _editorSession; | ||
|
||
private EditorSession editorSession; | ||
private IDocumentSymbolProvider symbolProvider; | ||
/// <summary> | ||
/// The symbol provider to get symbols from to build code lenses with. | ||
/// </summary> | ||
private IDocumentSymbolProvider _symbolProvider; | ||
|
||
/// <summary> | ||
/// Create a new Pester CodeLens provider for a given editor session. | ||
/// </summary> | ||
/// <param name="editorSession">The editor session context for which to provide Pester CodeLenses.</param> | ||
public PesterCodeLensProvider(EditorSession editorSession) | ||
{ | ||
this.editorSession = editorSession; | ||
this.symbolProvider = new PesterDocumentSymbolProvider(); | ||
_editorSession = editorSession; | ||
_symbolProvider = new PesterDocumentSymbolProvider(); | ||
} | ||
|
||
private IEnumerable<CodeLens> GetPesterLens( | ||
/// <summary> | ||
/// Get the Pester CodeLenses for a given Pester symbol. | ||
/// </summary> | ||
/// <param name="pesterSymbol">The Pester symbol to get CodeLenses for.</param> | ||
/// <param name="scriptFile">The script file the Pester symbol comes from.</param> | ||
/// <returns>All CodeLenses for the given Pester symbol.</returns> | ||
private CodeLens[] GetPesterLens( | ||
PesterSymbolReference pesterSymbol, | ||
ScriptFile scriptFile) | ||
{ | ||
var clientCommands = new ClientCommand[] | ||
var codeLensResults = new CodeLens[] | ||
{ | ||
new ClientCommand( | ||
"PowerShell.RunPesterTests", | ||
"Run tests", | ||
new object[] | ||
{ | ||
scriptFile.ClientFilePath, | ||
false, // Don't debug | ||
pesterSymbol.TestName, | ||
}), | ||
new CodeLens( | ||
this, | ||
scriptFile, | ||
pesterSymbol.ScriptRegion, | ||
new ClientCommand( | ||
"PowerShell.RunPesterTests", | ||
"Run tests", | ||
new object[] { scriptFile.ClientFilePath, false /* No debug */, pesterSymbol.TestName })), | ||
|
||
new ClientCommand( | ||
"PowerShell.RunPesterTests", | ||
"Debug tests", | ||
new object[] | ||
{ | ||
scriptFile.ClientFilePath, | ||
true, // Run in debugger | ||
pesterSymbol.TestName, | ||
}), | ||
new CodeLens( | ||
this, | ||
scriptFile, | ||
pesterSymbol.ScriptRegion, | ||
new ClientCommand( | ||
"PowerShell.RunPesterTests", | ||
"Debug tests", | ||
new object[] { scriptFile.ClientFilePath, true /* Run in debugger */, pesterSymbol.TestName })), | ||
}; | ||
|
||
return | ||
clientCommands.Select( | ||
command => | ||
new CodeLens( | ||
this, | ||
scriptFile, | ||
pesterSymbol.ScriptRegion, | ||
command)); | ||
return codeLensResults; | ||
} | ||
|
||
/// <summary> | ||
/// Get all Pester CodeLenses for a given script file. | ||
/// </summary> | ||
/// <param name="scriptFile">The script file to get Pester CodeLenses for.</param> | ||
/// <returns>All Pester CodeLenses for the given script file.</returns> | ||
public CodeLens[] ProvideCodeLenses(ScriptFile scriptFile) | ||
{ | ||
var symbols = | ||
this.symbolProvider | ||
.ProvideDocumentSymbols(scriptFile); | ||
var lenses = new List<CodeLens>(); | ||
foreach (SymbolReference symbol in _symbolProvider.ProvideDocumentSymbols(scriptFile)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so, yes. Why do you ask? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. spoke to @SeeminglyScience about this. We're good |
||
{ | ||
if (symbol is PesterSymbolReference pesterSymbol) | ||
{ | ||
if (pesterSymbol.Command != PesterCommandType.Describe) | ||
{ | ||
continue; | ||
} | ||
|
||
var lenses = | ||
symbols | ||
.OfType<PesterSymbolReference>() | ||
.Where(s => s.Command == PesterCommandType.Describe) | ||
.SelectMany(s => this.GetPesterLens(s, scriptFile)) | ||
.Where(codeLens => codeLens != null) | ||
.ToArray(); | ||
lenses.AddRange(GetPesterLens(pesterSymbol, scriptFile)); | ||
} | ||
} | ||
|
||
return lenses; | ||
return lenses.ToArray(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. echo: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going to leave this one on the basis of avoiding some LINQ closures |
||
} | ||
|
||
/// <summary> | ||
/// Resolve the CodeLens provision asynchronously -- just wraps the CodeLens argument in a task. | ||
/// </summary> | ||
/// <param name="codeLens">The code lens to resolve.</param> | ||
/// <param name="cancellationToken"></param> | ||
/// <returns>The given CodeLens, wrapped in a task.</returns> | ||
public Task<CodeLens> ResolveCodeLensAsync( | ||
CodeLens codeLens, | ||
CancellationToken cancellationToken) | ||
|
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.
More LINQ to remove?
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.
Is this the one I put back in?
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, see #719 (comment)