-
Notifications
You must be signed in to change notification settings - Fork 408
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
Port convert anonymous class creation to lambda expression from JDT.UI #684
Conversation
test this please |
@yaohaizh can you maybe separate this PR into 2 parts, one were you copy the jdt.ui files and the other from @SummerSun, where the new feature is added to jdt.ls? 2 commits, 2 authors |
@fbricon I think this is totally @SummerSun's contribution and we are not in rush. We can wait for a CQ if required. Is this okay for you? Thanks a lot |
@@ -13,13 +13,20 @@ | |||
package org.eclipse.jdt.ls.core.internal.corrections; | |||
|
|||
import org.eclipse.jdt.core.IJavaModelMarker; | |||
import org.eclipse.jdt.core.compiler.CategorizedProblem; | |||
import org.eclipse.jdt.core.compiler.IProblem; | |||
import org.eclipse.jdt.core.dom.ASTNode; | |||
import org.eclipse.jdt.core.dom.CompilationUnit; | |||
import org.eclipse.jdt.core.dom.NodeFinder; | |||
|
|||
public class ProblemLocation implements IProblemLocation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those changes seem unnecessary, none of those new fields are read
@@ -278,6 +288,44 @@ private static boolean getExtractMethodProposal(IInvocationContext context, ASTN | |||
return true; | |||
} | |||
|
|||
private static boolean getConvertAnonymousClassCreationsToLambdaProposals(IInvocationContext context, ASTNode covering, Collection<CUCorrectionProposal> resultingCollections) { | |||
while (covering instanceof Name || covering instanceof Type || covering instanceof Dimension || covering.getParent() instanceof MethodDeclaration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
potential NPE when calling covering.getParent()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The covering parameter is checked from the getAssists function before passing HERE , and the function getConvertAnonymousClassCreationsToLambdaProposals is private used in the same scope. Is that ok?
@fbricon the same formatting in Che |
@svor thanks for checking. @SummerSun can you see if this can be fixed please? |
@svor could you please help check the format Fred used in the sample? The format is all right when the anonymous class creation has correct format. But it is not fixed when the indent is incorrect in the anonymous class creation. Eclipse Oxygen does fix them. Not sure what the behavior in che. |
@SummerSun i think the same behavior in che: |
Since @yaohaizh fixed the formatting issue, all we have left to do is submit a CQ for this PR, before merging |
return true; | ||
} | ||
|
||
Map<String, String> options = new Hashtable<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new HashMap
Waiting for approval of CQ 16791 |
The Technology PMC is currently voting against copying such "a small change" from jdt.ui, would rather prefer we refactored the code upstream into jdt.core. We need to evaluate the amount of work necessary for that to happen. |
@fbricon ok. then we will wait for the API ready and update this PR too |
@SummerSun @yaohaizh merging the required changes to jdt.core that would avoid code duplication here is taking much longer that necessary, sorry for that. |
@fbricon So here we'll keep the code as-is and merge it first, and then refactor later? |
exactly. The PMC is aware it's taking too long to get the required changes upstream first, so we sort of got a free pass. |
Signed-off-by: Yaohai Zheng <[email protected]>
@fbricon Updated. |
CQ was finally approved \o/ |
Fixes #658
Signed-off-by: Summer Sun [email protected]