-
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
Add document symbols for #region #1911
Conversation
Waiting for #1886 as I'd rather get merge conflicts here 🙂 |
Heh, ok @fflaten things are so cleaned up now I think you can do this again (much more easily!) |
12e63f8
to
bd28a73
Compare
For consistent behavior and easier maintenance
Rewrote to use ReferenceTable for caching, but still a WIP. Todo:
|
I'd keep using it, it'll make everything easier. We can add "region" as a symbol type and just exclude them where appropriate. |
Hm I see what you mean now, since it's not done via AST but simply RegEx, and is only useful for the current document...I'll ponder, I'm sure there's a good way to handle that. |
Adds support for outline nesting and breadcrumbs
Regions aren't a reference. Only useful as cosmetic/navigation symbol in current document.
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.
If you want to take a crack at what I'm contemplating, go for it! Otherwise I'll try to get to it later today.
src/PowerShellEditorServices/Services/Symbols/ReferenceTable.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/Symbols/SymbolsService.cs
Outdated
Show resolved
Hide resolved
{ | ||
internal static class RegionVisitor | ||
{ | ||
internal static void GetRegionsInDocument(ScriptFile file, Func<SymbolReference, AstVisitAction> action) |
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 like this a lot, makes reworking it much easier!
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.
@andschwa: I assume you meant the separate class.
Because the Func-parameter won't make much sense when moving out of ReferenceTable, would it? Unless you'd prefer Action<SymbolReference>
-parameter over IEnumerable return type.
src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentHighlightHandler.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/TextDocument/Handlers/HoverHandler.cs
Outdated
Show resolved
Hide resolved
test/PowerShellEditorServices.Test/Language/SymbolsServiceTests.cs
Outdated
Show resolved
Hide resolved
I was wrong here 😆 |
{ | ||
return null; | ||
} | ||
foundSymbols = foundSymbols.Concat(RegionVisitor.GetRegionsInDocument(scriptFile)); | ||
|
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 didn't see a reason to keep the null-check. The local method ProvideDocumentSymbols()
always returns a list, right?
@@ -952,5 +959,30 @@ public void FindsSymbolsInNoSymbolsFile() | |||
IEnumerable<SymbolReference> symbolsResult = FindSymbolsInFile(FindSymbolsInNoSymbolsFile.SourceDetails); | |||
Assert.Empty(symbolsResult); | |||
} | |||
|
|||
[Fact] | |||
public void FindsRegionSymbolsInFile() |
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.
This technically doesn't belong here as SymbolsService isn't used for DocumentSymbolHandler. Move to own test-file? Suggestion?
Closing since I integrated this, thanks again so much for your work! |
PR Summary
Adds symbols for regions for easier navigation. Currently available in outline + breadcrumbs and "go to symbol in current editor".
PR Context
Fix PowerShell/vscode-powershell#3604
Fix PowerShell/vscode-powershell#3210