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

Fixes #517 Implement Code Action Participant in HttpServletQuickFix #532

Conversation

Rahul-Biju
Copy link
Contributor

@Rahul-Biju Rahul-Biju commented Oct 16, 2023

Fixes #517 Implement Code Action Participant in HttpServletQuickFix.

@aparnamichael aparnamichael changed the base branch from main to code-action-unification October 16, 2023 13:04
@Rahul-Biju Rahul-Biju changed the title Fixes #24 Implement Code Action Participant in HttpServletQuickFix Fixes #517 Implement Code Action Participant in HttpServletQuickFix Oct 16, 2023
public List<? extends CodeAction> getCodeActions(JavaCodeActionContext context, Diagnostic diagnostic) {
List<CodeAction> codeActions = new ArrayList<>();
PsiElement node = context.getCoveredNode();
PsiClass parentType = getBinding(node);
if (parentType != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This null check is required. Please add this back to the code so that an empty list of code actions is returned if there is no parent type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrglavas addressed the review comments.

Comment on lines 63 to 68
String newText = "package io.openliberty.sample.jakarta.servlet;\n\n" +
"import jakarta.servlet.annotation.WebServlet;\nimport jakarta.servlet.http.HttpServlet;\n\n" +
"@WebServlet(name = \"demoServlet\", urlPatterns = {\"/demo\"})\n" +
"public class DontExtendHttpServlet extends HttpServlet {\n\n}";

TextEdit te = JakartaForJavaAssert.te(0, 0, 7, 1, newText);
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't seem to have enabled this portion of the test in your PR. I can see it's still inside a block of code that won't be executed. I would suggest looking at some of the other PRs to see how others have been enabling tests for code actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrglavas addressed the review comments.

Copy link
Contributor

@mrglavas mrglavas left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks.

Copy link
Contributor

@aparnamichael aparnamichael left a comment

Choose a reason for hiding this comment

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

Looks good.

group="jakarta"
targetDiagnostic="jakarta-cdi#InvalidManagedBeanConstructor"
implementationClass="io.openliberty.tools.intellij.lsp4jakarta.lsp4ij.cdi.ManagedBeanNoArgConstructorQuickFix"/>

Copy link
Contributor

Choose a reason for hiding this comment

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

are these quick fixes intended to be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TrevCraw addressed the review comments.

@TrevCraw TrevCraw requested a review from mrglavas October 20, 2023 06:11
@Rahul-Biju Rahul-Biju merged commit a737c10 into OpenLiberty:code-action-unification Oct 20, 2023
2 of 3 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
4 participants