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

Port convert anonymous class creation to lambda expression from JDT.UI #684

Merged
merged 1 commit into from
Sep 4, 2018
Merged

Conversation

SummerSun
Copy link

@SummerSun SummerSun commented Jun 7, 2018

Fixes #658

Signed-off-by: Summer Sun [email protected]

@fbricon
Copy link
Contributor

fbricon commented Jun 7, 2018

test this please

@fbricon
Copy link
Contributor

fbricon commented Jun 7, 2018

@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

@yaohaizh
Copy link
Contributor

yaohaizh commented Jun 7, 2018

@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

@fbricon
Copy link
Contributor

fbricon commented Jun 7, 2018

It works as expected but the formatting/indentation seems off in vscode:
jun-07-2018 11-15-14

@svor / @tolusha can you check how it behaves in che?

@@ -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 {
Copy link
Contributor

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
Copy link
Contributor

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()

Copy link
Author

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?

@svor
Copy link
Contributor

svor commented Jun 8, 2018

@fbricon the same formatting in Che
output

@fbricon
Copy link
Contributor

fbricon commented Jun 8, 2018

@svor thanks for checking. @SummerSun can you see if this can be fixed please?

@SummerSun
Copy link
Author

SummerSun commented Jun 11, 2018

@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.

lambda

@svor
Copy link
Contributor

svor commented Jun 11, 2018

@SummerSun i think the same behavior in che:
l

@fbricon fbricon mentioned this pull request Jun 18, 2018
26 tasks
@fbricon
Copy link
Contributor

fbricon commented Jun 20, 2018

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<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

new HashMap

@fbricon
Copy link
Contributor

fbricon commented Jun 21, 2018

Waiting for approval of CQ 16791

@fbricon
Copy link
Contributor

fbricon commented Jun 25, 2018

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
Copy link
Contributor

fbricon commented Jun 28, 2018

so @jjohnstn and @rgrunber are currently working on refactoring/exposing the necessary APIs from jdt.core that will be required for this PR (among other things).
See

@SummerSun
Copy link
Author

@fbricon ok. then we will wait for the API ready and update this PR too

@fbricon
Copy link
Contributor

fbricon commented Aug 27, 2018

@SummerSun @yaohaizh merging the required changes to jdt.core that would avoid code duplication here is taking much longer that necessary, sorry for that.
Can you please rebase your PR against master ASAP and we'll restart the CQ process, as we were guaranteed PMC approval this time. Doing it before the next release (Aug 31st) would be ideal.

@yaohaizh
Copy link
Contributor

@fbricon So here we'll keep the code as-is and merge it first, and then refactor later?

@fbricon
Copy link
Contributor

fbricon commented Aug 28, 2018

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.

@yaohaizh
Copy link
Contributor

@fbricon Updated.

@fbricon fbricon merged commit 70e96c4 into eclipse-jdtls:master Sep 4, 2018
@fbricon
Copy link
Contributor

fbricon commented Sep 4, 2018

CQ was finally approved \o/
Sorry it took that long, but we can finally move forward now.
@SummerSun @yaohaizh thanks for your help and patience

@SummerSun SummerSun deleted the qisun_dev branch September 5, 2018 00:46
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.

4 participants