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

Implement method hierarchy through existing type hierarchy logic. #2502

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

rgrunber
Copy link
Contributor

@rgrunber rgrunber commented Feb 28, 2023

  • The method hierarchy for a given method of some type is a subset of the type hierarchy where methods not implemented are filtered out

method-hierarchy

  • I should add some test cases for this

@CsCherrYY
Copy link
Contributor

Just played with the change and LGTM overall. Some questions:

  1. Can we consider to change the view title? Currently it's always Class Hierarchy, since the client is able to know whether it's a method hierarchy, a title like Method hierarchy for ${methodName} would be better I think.
  2. Should we show the supertypes of the first type who implement the method? e.g., type Object in your example. I tried in IntelliJ, Object is now shown there.
    image

@rgrunber
Copy link
Contributor Author

rgrunber commented Mar 13, 2023

  1. The display is done purely on the client side, so I modified that part to show Method Hierarchy or Subtype (Method) Hierarchy / Super (Method) Hierarchy when that's the case. In order to also display the method name I'll just need to add an extra field under data. the current method data is just a unique identifier to retrieve the IMethod but it can't really be nicely used to retrieve a human-readable name.
  2. I've excluded java.lang.Object, except for the special case where the method being searched originates from there (eg. toString, equals, etc.). In those cases I think it's worth showing just for the sake of correctness.

Update: I've sent an additional method_name data field that contains a human-readable method name. It might be possible to parse from the handle identifier since the method name seems to be contained within the tilde (~) char, but I'm not sure if that always holds true. See https://github.com/eclipse-jdt/eclipse.jdt.core/blob/c4a6546b03ec2be75940407d0c583d8c0c0ad2f3/org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/SourceMethod.java#L87-L94 .

@CsCherrYY
Copy link
Contributor

As for the human-readable name part, since we can get the IMethod either from the handle identifier or getMember(), how about directly using IMethod.getElementName()?

@rgrunber
Copy link
Contributor Author

As for the human-readable name part, since we can get the IMethod either from the handle identifier or getMember(), how about directly using IMethod.getElementName()?

Yup, that's exactly what I've gone with (forgot to push the change).

Copy link
Contributor

@CsCherrYY CsCherrYY left a comment

Choose a reason for hiding this comment

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

LGTM!

- The method hierarchy for a given method of some type is a subset of
  the type hierarchy where methods not implemented are filtered out
- Add testcase

Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
@rgrunber rgrunber merged commit 782ca15 into eclipse-jdtls:master Mar 15, 2023
@rgrunber rgrunber deleted the method-hierarchy branch March 15, 2023 20:08
@rgrunber rgrunber added this to the Mid March 2023 milestone Mar 15, 2023
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.

2 participants