-
Notifications
You must be signed in to change notification settings - Fork 51
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
add quickfix fn to handle composite annotations #513
Conversation
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.
Hi @ajm01, the code works great. I just had a few comments.
} | ||
} | ||
if (a instanceof SingleMemberAnnotation) { | ||
// this type of annotation contains a single valuie which may be a list of other annotations, |
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.
Typo: "valuie"
@@ -19,19 +19,27 @@ | |||
import java.util.List; | |||
|
|||
import org.eclipse.core.runtime.CoreException; | |||
import org.eclipse.jdt.core.IAnnotation; |
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.
Update copyright year to 2024 in this file and others.
|
||
newSingleMemberAnnotation.setTypeName( | ||
ast.newName(imports.addImport(annotationToProcess.getTypeName().toString(), importRWCtx))); | ||
List<NormalAnnotation> newCompositeAnnotationContents = newAIInstance.expressions(); |
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.
It looks like you populate this: newCompositeAnnotationContents, but it is never used.
} | ||
|
||
// add new String attributes | ||
//values = addNewAttributes(ast, values); |
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.
Remove this commented line.
} | ||
|
||
// process methods now? |
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.
Remove: // process methods now?
|
||
@Resource(type = Object.class, name = "aa") | ||
@Resources ({ @Resource(name = "aaa"), @Resource(type = Object.class) }) |
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.
A couple of things here:
- You may want to format this file.
- Why replace @resource for @Resources? That could be used for a positive test where we check that no diagnostics are generated because everything looks fine.
- I would suggest that you create a ResourcesAnnotation class and add a similar set of code for a similar set of diagnostics/code-action tests including a positive test.
e2240f8
to
14793a0
Compare
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.
Andy, you missed a few copyrights.
@@ -28,6 +28,7 @@ public enum JakartaCodeActionId implements ICodeActionId { | |||
ChangeReturnTypeToVoid, | |||
InsertResourceAnnotationTypeAttribute, | |||
InsertResourceAnnotationNameAttribute, |
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.
Update Copyright year to 2024
@@ -30,6 +30,10 @@ public class Constants { | |||
public static final String RESOURCE = "Resource"; | |||
public static final String RESOURCE_FQ_NAME = "jakarta.annotation.Resource"; | |||
|
|||
/* @Resources */ |
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.
Update Copyright year to 2024
@@ -18,6 +18,7 @@ MethodMustNotHaveParameters = A method with the {0} annotation must not have any | |||
MethodMustBeVoid = A method with the {0} annotation must be void. | |||
MethodMustNotBeStatic = A method with the {0} annotation must not be static. | |||
MethodMustNotThrow = A method with the {0} annotation must not throw checked exceptions. | |||
ResourcesAnnotationMustDefineResourceAnnotation = The {0} annotation must define at least one sub-annotation ''{1}''. |
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.
Update Copyright year to 2024
@@ -0,0 +1,78 @@ | |||
/******************************************************************************* |
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.
Since this is a new file the first like should be:
Copyright (c) 2024 IBM Corporation and others.
fixes #190