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 symbols for class-types and configuration #1886

Closed
wants to merge 18 commits into from

Conversation

fflaten
Copy link
Contributor

@fflaten fflaten commented Aug 13, 2022

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:

TODO:

  • Manual test
  • 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)

@ghost ghost added Area-Code Navigation Issue-Enhancement A feature request (enhancement). labels Aug 13, 2022
@fflaten
Copy link
Contributor Author

fflaten commented Aug 14, 2022

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?

@JustinGrote
Copy link
Collaborator

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!

@SydneyhSmith
Copy link
Collaborator

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!

@fflaten
Copy link
Contributor Author

fflaten commented Aug 16, 2022

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

Ex.

  • Class-related document symbols (outline + workspace symbol search)
  • 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.

@SeeminglyScience
Copy link
Collaborator

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.

@fflaten
Copy link
Contributor Author

fflaten commented Aug 17, 2022

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 fflaten changed the title Add script symbols for class-types Add symbols for class-types and configuration Aug 18, 2022
@fflaten fflaten marked this pull request as ready for review August 25, 2022 20:05
@fflaten fflaten requested a review from a team August 25, 2022 20:05
@fflaten
Copy link
Contributor Author

fflaten commented Aug 25, 2022

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.

@andyleejordan
Copy link
Member

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
Copy link
Member

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

@fflaten
Copy link
Contributor Author

fflaten commented Nov 11, 2022

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.

@andyleejordan andyleejordan self-assigned this Dec 1, 2022
@andyleejordan
Copy link
Member

Merged by #1984

@fflaten fflaten deleted the class-symbols branch February 2, 2023 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outline, symbol search and go to definition should jump to symbol name Classes should be symbols
5 participants