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 interface and abstract method implementation code lens #3333

Merged

Conversation

hopehadfield
Copy link
Contributor

@hopehadfield hopehadfield commented Nov 26, 2024

@rgrunber
Copy link
Contributor

So this just works because the underlying code supported it all along ?! Just widening a few parameters..

if (javaElement instanceof IMethod) {
implementations = findMethodImplementations(monitor);
} else if (javaElement instanceof IType) {
implementations = findTypeImplementations(monitor);
}

@rgrunber rgrunber self-requested a review November 27, 2024 04:41
@rgrunber rgrunber added this to the Mid December 2024 milestone Nov 28, 2024
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Overall, this looks good and works well. The only thing I have to consider is whether we care to be backwards compatible in the language server also 😜

Also, I found the implementation behaviour even for types (so independent of this PR) diverges from how Eclipse does it. For example, Eclipse will show as implementations, methods/types that override/extend non-interface, non-abstract classes. However, because of that isInterface / isAbstract logic prior to calling getCodeLens(..) we completely avoid rendering such classes. Might need to look back and see if there's a good reason we did this. If not, we could fix it in a later PR.

@rgrunber
Copy link
Contributor

rgrunber commented Dec 6, 2024

... The only thing I have to consider is whether we care to be backwards compatible in the language server also 😜

I've decided it's not worth doing this on the language server side. It's probably enough if we just mention the rename in the eventual changelog.

@hopehadfield hopehadfield force-pushed the 3813-method-implementations branch 2 times, most recently from c72b804 to 660f67f Compare December 10, 2024 23:14
Copy link
Contributor

@rgrunber rgrunber 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. Can you just remove those extraneous whitespaces (tabs) on the new lines in the 3 test classes. Ext, ITest & Test ? git show displays them.

@hopehadfield hopehadfield force-pushed the 3813-method-implementations branch from 660f67f to 6759b15 Compare December 16, 2024 18:31
@rgrunber rgrunber merged commit 3a6cd74 into eclipse-jdtls:master Dec 17, 2024
6 of 7 checks passed
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.

implementationsCodeLens support for method implementations
2 participants