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

Sort the quick fix menu for imports in a order that makes sense #550

Merged
merged 1 commit into from
Feb 15, 2018

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Feb 13, 2018

Fixes redhat-developer/vscode-java#424

Signed-off-by: Snjezana Peco snjezana.peco@redhat.com

@snjeza snjeza requested review from gorkem and fbricon February 13, 2018 16:45
Copy link
Contributor

@fbricon fbricon left a comment

Choose a reason for hiding this comment

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

Rather than expecting a different, seemingly random, order like assertCodeActions(cu, e3, e1, e2, e4); in those tests assertions, I'd rather keep the assertions intact and rearrange the Expected instance declarations instead.

I understand it's more tedious to do now, but it will be more maintainable on the long term

@@ -133,4 +136,19 @@ private static CompilationUnit getASTRoot(ICompilationUnit unit) {
return SharedASTProvider.getInstance().getAST(unit, new NullProgressMonitor());
}

class CUCorrectionProposalComparator implements Comparator<CUCorrectionProposal> {
Copy link
Contributor

Choose a reason for hiding this comment

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

private static class

@fbricon
Copy link
Contributor

fbricon commented Feb 13, 2018

It doesn't work properly for InputStream for instance:

screen shot 2018-02-13 at 12 32 27 pm

Compared to Eclipse:
screen shot 2018-02-13 at 12 35 50 pm

@snjeza
Copy link
Contributor Author

snjeza commented Feb 13, 2018

It doesn't work properly for InputStream for instance:

I can't reproduce the issue.

424

@fbricon
Copy link
Contributor

fbricon commented Feb 13, 2018

I'm definitely getting a different sort. And I rebuilt the server 3 times. I need to figure out why. FWIW, I'm using the latest vscode insiders build (1.21.0-insider 00f2d8ed7196606d31cc1759a171486d1cc00deb)

@snjeza
Copy link
Contributor Author

snjeza commented Feb 13, 2018

The issue can be reproduced when using VS Code >= 1.20
I suppose it is caused by microsoft/vscode#34664

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>
@snjeza
Copy link
Contributor Author

snjeza commented Feb 14, 2018

@fbricon I have updated the PR.

@fbricon fbricon merged commit 71e2d76 into eclipse-jdtls:master Feb 15, 2018
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