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 support for type hierarchy requests #2103

Merged
merged 9 commits into from
Jun 18, 2024
Merged

Add support for type hierarchy requests #2103

merged 9 commits into from
Jun 18, 2024

Conversation

Morriar
Copy link
Contributor

@Morriar Morriar commented May 29, 2024

Closes #1046.

Implement support for:

Demo:

demo

@Morriar Morriar added the enhancement New feature or request label May 29, 2024
@Morriar Morriar self-assigned this May 29, 2024
@Morriar Morriar requested a review from a team as a code owner May 29, 2024 21:08
@Morriar Morriar requested review from andyw8 and vinistock May 29, 2024 21:08
@vinistock vinistock force-pushed the vs/add_ancestors_linearization branch 3 times, most recently from 401c668 to 9566119 Compare May 30, 2024 17:59
Base automatically changed from vs/add_ancestors_linearization to main May 30, 2024 18:13
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a rebase, but this looks good to me.

We should also add the typeHierarchy configuration and its respective default here so that it's enabled.

@Morriar Morriar force-pushed the atvs-types-hierarchy branch 2 times, most recently from 7c602b8 to 8adf61e Compare June 6, 2024 18:04
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. For whatever reason, switching between sub and super types multiple times on certain namespaces causes an error in the extension side Cannot read properties of undefined (reading 'map').

You can reproduce it in the core_ext/uri file. It succeeds initially, but switching back and forth eventually fails.

I'm not sure if this isn't because of the lack of sub types implementation, so maybe we can move forward and debug that after sub types?

@vinistock vinistock added the server This pull request should be included in the server gem's release notes label Jun 6, 2024
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the 2 gifs are exactly the same?

@Morriar
Copy link
Contributor Author

Morriar commented Jun 6, 2024

Looks good. For whatever reason, switching between sub and super types multiple times on certain namespaces causes an error in the extension side Cannot read properties of undefined (reading 'map').

Good find, I can investigate 👍

@Morriar
Copy link
Contributor Author

Morriar commented Jun 6, 2024

I think the 2 gifs are exactly the same?

They are, the prepareTypeHierarchy doesn't do much, it just prepares the field for the supertypes / subtypes requests.

The issue is that we test for each request to have a gif and for the gif to be the same name than the request. I guess I could use a symlink though?

@Morriar Morriar force-pushed the atvs-types-hierarchy branch 2 times, most recently from 63e8856 to ea82137 Compare June 14, 2024 16:02
@Morriar
Copy link
Contributor Author

Morriar commented Jun 14, 2024

I updated the PR to changes two behaviors:

  1. Always return only one item in the prepareTypeHierarchy response to avoid a VSCode crash while we investigate what should be the expected behavior
  2. Only return the first level of direct ancestors rather than the fully linearized list while we decide what is the best DX

@Morriar Morriar requested review from st0012 and vinistock June 14, 2024 16:04
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

parent = context.parent
return unless node && parent

target = determine_target(node, parent, @position)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but I think we can make determine_target simply take a NodeContext because all of its usages take node and parent from the same context.

lib/ruby_lsp/requests/prepare_type_hierarchy.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/requests/type_hierarchy_supertypes.rb Outdated Show resolved Hide resolved
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar merged commit a8a03f0 into main Jun 18, 2024
32 checks passed
@Morriar Morriar deleted the atvs-types-hierarchy branch June 18, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add type hierarchy support
3 participants