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.

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

Closed
wants to merge 10 commits into from
Closed

Conversation

fflaten
Copy link
Contributor

@fflaten fflaten commented Aug 26, 2022

PR Summary

Adds symbols for regions for easier navigation. Currently available in outline + breadcrumbs and "go to symbol in current editor".

image

PR Context

Fix PowerShell/vscode-powershell#3604
Fix PowerShell/vscode-powershell#3210

@fflaten
Copy link
Contributor Author

fflaten commented Aug 26, 2022

Waiting for #1886 as I'd rather get merge conflicts here 🙂

@andyleejordan
Copy link
Member

Heh, ok @fflaten things are so cleaned up now I think you can do this again (much more easily!)

For consistent behavior and easier maintenance
@fflaten
Copy link
Contributor Author

fflaten commented Feb 14, 2023

Rewrote to use ReferenceTable for caching, but still a WIP.
@andschwa: Does it make sense to use ReferenceTable here? It'll make it available for handlers incl. find all references. And it isn't something we'd need to track in the workspace.

Todo:

  • Test with full scriptrange (track endregion)
    • How does outline behave when endregion occurs in the middle of a function/class etc?
  • Validate regex behavior against PowerShell ISE
  • Move out of ReferenceTable?
  • Ignore symbol in scenarios like highlight and hover?

@andyleejordan
Copy link
Member

Does it make sense to use ReferenceTable here?

I'd keep using it, it'll make everything easier. We can add "region" as a symbol type and just exclude them where appropriate.

@andyleejordan
Copy link
Member

Does it make sense to use ReferenceTable here?

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.
@fflaten
Copy link
Contributor Author

fflaten commented Feb 18, 2023

  • Test with full scriptrange (track endregion)

    • How does outline behave when endregion occurs in the middle of a function/class etc?

It didn't break as I feared, but only wraps symbols with both start and end position inside. Ex.
image

Updated to include it and returning only regions with matching end just like folding.
I've disabled hover, highlight (on click) and reference scanning.

Hopefully we'll get some feedback if it becomes too noisy in go to symbol or the outline-nesting. Breadcrumbs require the latter to work, so hopefully worth it.

@fflaten fflaten marked this pull request as ready for review February 18, 2023 12:57
@fflaten fflaten requested a review from a team February 18, 2023 12:57
Copy link
Member

@andyleejordan andyleejordan left a 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.

{
internal static class RegionVisitor
{
internal static void GetRegionsInDocument(ScriptFile file, Func<SymbolReference, AstVisitAction> action)
Copy link
Member

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!

Copy link
Contributor Author

@fflaten fflaten Feb 24, 2023

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.

@andyleejordan
Copy link
Member

Does it make sense to use ReferenceTable here?

I'd keep using it, it'll make everything easier. We can add "region" as a symbol type and just exclude them where appropriate.

I was wrong here 😆

{
return null;
}
foundSymbols = foundSymbols.Concat(RegionVisitor.GetRegionsInDocument(scriptFile));

Copy link
Contributor Author

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()
Copy link
Contributor Author

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?

@andyleejordan
Copy link
Member

Closing since I integrated this, thanks again so much for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outline should also support #Region Include regions in symbol list/outline/breadcrumbs
2 participants