-
Notifications
You must be signed in to change notification settings - Fork 222
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
Integrating class symbol support #1984
Conversation
82ec11f
to
604d6a7
Compare
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.
Some quick comments but there's more, I just need to eat lunch.
src/PowerShellEditorServices/Services/Symbols/ReferenceTable.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/Symbols/ReferenceTable.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/Symbols/ReferenceTable.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/Symbols/ReferenceTable.cs
Outdated
Show resolved
Hide resolved
This is coming along very well! I'll try to consolidate my thoughts in this PR 🙂
|
Latest commit fixed the reference-issue with large files 🎉 I've updated the previous post |
Since `FindReferencesOfSymbol` is already used.
This required using a `ScriptFile` for the associated test in `AstOperationsTests`. The coupling of the visitor to the file seems fine since that's the _point_ of this improvement: a cached sets of references for each file. We also have to carefully sort our list of symbol references for things that expect them in order.
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'm probably about 2/3's done reviewin' but here's what I got so far. Very exciting changes though! ❤️
src/PowerShellEditorServices/Services/Symbols/SymbolsService.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/TextDocument/ScriptRegion.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/TextDocument/ScriptRegion.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/TextDocument/ScriptRegion.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Patrick Meinecke <SeeminglyScience@users.noreply.github.com>
@@ -89,7 +89,7 @@ private static ParameterInformation CreateParameterInfo(ParameterInfo parameterI | |||
return new ParameterInformation | |||
{ | |||
Label = parameterInfo.Name, | |||
Documentation = string.Empty | |||
Documentation = parameterInfo.HelpMessage |
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.
@SeeminglyScience Not a whole lot comes from this it seems.
So that we rely on the dictionary to sort our symbols into the equivalent types instead of having to perform a filter.
I think there's a cleaner way to do this, but this works.
Against the full name and not just the identifier, since it's what's displayed in the search menu. Now you can search methods by their parameters' names.
src/PowerShellEditorServices/Services/Symbols/Visitors/SymbolVisitor.cs
Outdated
Show resolved
Hide resolved
src/PowerShellEditorServices/Services/Symbols/Visitors/SymbolVisitor.cs
Outdated
Show resolved
Hide resolved
Should have been just `IndexOf`, and also use `+` instead of `string.Concat()` for simplicity and speed.
Thanks @fflaten and @SeeminglyScience, resolved those things! |
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 is the full (admittedly still a bit of a work-in-progress PR) that integrates #1886 into
main
.