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

Make CoreJavadocAccessImpl more extendable for jdt.ls #1386

Merged

Conversation

robstryker
Copy link
Contributor

What it does

#1351 seems to have fixed an inadvertent change to the value of some constants in a file that was being abstracted out for subclassing.

It's clear that the abstraction process was incomplete and only appeared complete because of these changed constants. The reversion of those constants to their historical value has revealed that jdt.ls cannot override the behavior as expected.

In an attempt to allow jdt.ls to override these values, this PR will make getters for the 6 values, which subclasses may override. All references to those constants in this file can be changed to make use of the getters instead.

This should allow jdt.ls more flexibility in overriding these values and using different formatting as they require.

How to test

This commit should not change current behavior. The test as performed in #1351 should be sufficient to see that no changes in behavior are observed.

Author checklist

@robstryker
Copy link
Contributor Author

@iloveeclipse Hi Andrey: Your feedback on this issue is welcome / appreciated.

@iloveeclipse
Copy link
Member

Hi Andrey: Your feedback on this issue is welcome / appreciated.

Looks OK, please just make sure search/replace hit the right places :)

@robstryker
Copy link
Contributor Author

If I comment out the declaration of the constants, the only errors are in those getters, so, looks like I didn't miss any uses.

Verified line by line that the proper replacements were made, and that the getters return the correct constant.

@iloveeclipse iloveeclipse merged commit 38c388e into eclipse-jdt:master May 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants