Expose Fix Imports command to LSP Clients#9062
Conversation
lahodaj
left a comment
There was a problem hiding this comment.
Overall seems reasonable to me, with some suggestions for small cleanups inline.
| /** Creates a new instance of JavaFixAllImports */ | ||
| private JavaFixAllImports() { | ||
| } | ||
|
|
| */ | ||
| public abstract class CodeActionsProvider { | ||
|
|
||
| public static final String FIX_IMPORTS_KIND = "source.fixImports"; |
There was a problem hiding this comment.
I suspect this should be moved to java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/FixImportsCodeAction.java? (I think that's consistent with what is being done for other ordinary implementations of CodeActionsProviders.
| import org.eclipse.lsp4j.CodeActionKind; | ||
| import org.eclipse.lsp4j.CodeActionParams; | ||
| import org.eclipse.lsp4j.TextEdit; | ||
| import org.eclipse.lsp4j.WorkspaceEdit; |
There was a problem hiding this comment.
The changes in this file do not seem to be necessary - these can be removed.
fe4b915 to
322b794
Compare
lahodaj
left a comment
There was a problem hiding this comment.
Overall, looks reasonable to me, with some comments for consideration. Thanks for doing this!
| codeAction.setEdit(new WorkspaceEdit(Collections.singletonMap(uri, edits))); | ||
| } | ||
| } catch (IOException ex) { | ||
| Exceptions.printStackTrace(ex); |
There was a problem hiding this comment.
Not sure if this is intentional, but unless there's a good reason, I would simply let the exception be thrown from thenApply, which should complete the CompletableFuture exceptionally. E.g. throw new IllegalStateException(ex);. That way, the UI may have an opportunity to show a sensible error to the user.
| } | ||
| } catch (IOException ex) { | ||
| Exceptions.printStackTrace(ex); | ||
|
|
There was a problem hiding this comment.
Nit: I would suggest to delete this empty line. (I guess more generally, empty lines before closing braces, esp. inside methods.)
| @Function | ||
| void completeSelectedCandidates(FixImportsUI ui) { | ||
| List<ImportDataUI> imports = ui.getImports(); | ||
| CandidateDescription[] choosen = IntStream.range(0, imports.size()) |
| @Property(name = "simpleName", type = String.class), | ||
| @Property(name = "selectedCandidateFQN", type = String.class), | ||
| @Property(name = "candidatesFQN", type = String.class, array = true) | ||
|
|
There was a problem hiding this comment.
Nit: I would remove this empty line.
| src.getParentFile().mkdirs(); | ||
| try (Writer w = new FileWriter(new File(src.getParentFile().getParentFile(), ".test-project"))) { | ||
| } | ||
| String code = "package a;\n" |
There was a problem hiding this comment.
Nit: java.lsp.server can use JDK 17 features now, could be replaced with text blocks, for better readability.
322b794 to
750044c
Compare
|
Thanks again @lahodaj !! |
|
CI found something, looks like a test doesn't compile anymore: |
750044c to
1baf2f4
Compare
|
Thanks @mbien !! , for pointing this one out . It was due to the upgrade of lsp4j where the old constructor was removed . Have fixed it now. |
|
I there are no objections, I'll merge later today or tomorrow. |
Fix Imports Code action for LSP Client
Fix Imports...as a source code action to add all missing imports in one command.Click to collapse/expand PR instructions
By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -
Please make sure (eg.
git log) that all commits have a valid name and email address for you in the Author field.If you're a first time contributor, see the Contributing guidelines for more information.
If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.
PR approval and merge checklist:
If this PR targets the delivery branch: don't merge. (full wiki article)