PowerShellEditorServices icon indicating copy to clipboard operation
PowerShellEditorServices copied to clipboard

Add symbols for class-types and configuration

Open fflaten opened this issue 3 years ago • 8 comments

PR Summary

Adds support for class-related symbols:

  • Class
  • Enum
  • Property and EnumMember (no references)
  • Methods (no references)
  • Constructors (no references)
  • Configuration (no references/definition and highlight)

All symbol types have document symbols (outline + symbol search), symbol highlight and hover. Only class and enum has code lense and references.

Methods and Constructors are shown with parameters in outline + workspace symbol search.

Also:

  • Fixes symbol-detection when newline between function-keyword and name (#1905)
  • Comment and code cleanup in relevant parts

TODO:

  • [x] Manual test
  • [x] Add tests

PR Context

Easier navigation, search and sticky scroll.

Fix #1683 Fix #1905 Related #14 (fixed also? what is parameter hints?) Related PowerShell/vscode-powershell#3 (missing references for class members)

fflaten avatar Aug 13 '22 22:08 fflaten

Do you prefer this PR being solely document symbols for outline/workspace search as a first step to keep PR size small, like in PowerShell-repo? Or should everything be implemented before merge and release? I personally just needed documentsymbols for a project.

I planned to implement hover and highlight quickly, but highlight requires FindReferencesVistor also. So in the end I'd need to implement references/declaration and PR will grow + take time to figure out how to test it all. 😄

Also just to make sure before I waste any time, @JustinGrote are you or someone else working on this already?

fflaten avatar Aug 14 '22 15:08 fflaten

Also just to make sure before I waste any time, @JustinGrote are you or someone else working on this already?

Nope I am not, this would be very welcome!

JustinGrote avatar Aug 14 '22 15:08 JustinGrote

Do you prefer this PR being solely document symbols for outline/workspace search as a first step to keep PR size small, like in PowerShell-repo? Or should everything be implemented before merge and release? I personally just needed documentsymbols for a project.

I planned to implement hover and highlight quickly, but highlight requires FindReferencesVistor also. So in the end I'd need to implement references/declaration and PR will grow + take time to figure out how to test it all. 😄

Also just to make sure before I waste any time, @JustinGrote are you or someone else working on this already?

We generally probably prefer smaller PRs scoped to complete work items, however it depends on exactly what the scope of work is...what PRs/work items would you imagine being related to this work item?

Thanks!

SydneyhSmith avatar Aug 16 '22 18:08 SydneyhSmith

what PRs/work items would you imagine being related to this work item?

Ex.

  • Class-related document symbols (outline + workspace symbol search)
    • This PR + cleanup and tests - would consider it as a fix for #1683
  • Class references/declarations (go to .. and codelense)
  • Class-related symbol hover and highlight (on click)

Not sure if I can split any more because the AST-visitors are somewhat mixed together in some of the providers, so solve one -> solve many.

I'm already working on the remaining functionality in #14, so just wondering if I should start to consider how/if I can split it or if it's fine to let this PR grow.

fflaten avatar Aug 16 '22 19:08 fflaten

Thank you so much for gettin' the ball rollin' on this ❤️

I planned to implement hover and highlight quickly, but highlight requires FindReferencesVistor also

Just a heads up that the engine doesn't really provide any sort of static type/member inference atm. So anything that requires tracking references might need to be deferred for a bit.

SeeminglyScience avatar Aug 17 '22 16:08 SeeminglyScience

Just a heads up that the engine doesn't really provide any sort of static type/member inference atm. So anything that requires tracking references might need to be deferred for a bit.

I assume you're meaning properties, methods and constructors?

I only plan on tracking references for class/enum definitions against type constraints and expressions ([typeName]).

Custom classes are hopefully referenced with Name that is unique and not qualified. Same risk as duplicate functions in different scopes which PSES already get mixed.

image

fflaten avatar Aug 17 '22 17:08 fflaten

I'd like to start by apologizing for the mess 😄

I should've stopped at document symbols so I could have multiple smaller PRs. Highlight basically required everything implemented at which point it made sense to refactor some code to avoid repeating it + avoid inconsistent symbol start-position (keyword vs name).

Ready for review. Not sure what's going on with the CodeQL check. MacOS failure is flaky test.

fflaten avatar Aug 25 '22 20:08 fflaten

I love it, but am going to wait for the next release as we'll want to put this through a preview for a bit!

andyleejordan avatar Aug 25 '22 20:08 andyleejordan

OMG @fflaten I had so much going on that this fell off my radar. I'm so sorry!

andyleejordan avatar Oct 24 '22 23:10 andyleejordan

OMG @fflaten I had so much going on that this fell off my radar. I'm so sorry!

It happens, no worries 🙂 Would welcome some feedback as I assume there's parts that needs to be updated now.

fflaten avatar Nov 11 '22 20:11 fflaten

Merged by https://github.com/PowerShell/PowerShellEditorServices/pull/1984

andyleejordan avatar Feb 02 '23 20:02 andyleejordan